-
Notifications
You must be signed in to change notification settings - Fork 71
New CSR protocal from the compiler with separated start registers #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@pcolberg @zibaiwan This PR currently breaks runtime backwards compatibility since we added new registers to the CSR and the addresses are shifted. How should we maintain the runtime backwards compatibility going forward? The runtime must first query the CSR version before setting up the proper addresses to the registers. How would we do that? I'm thinking of maybe adding an offset to the address once the CSR version number is set. What are your thoughts? |
Hi @ericxu233,
Currently the runtime is already queries the CSR version before read/write into the CSR at here. It is set in the
Currently the runtime support two different CSR versions. The 4 one is the latest one created by the compiler. through the code, you should be able to see that the runtime does something different for the older CSR version 3.
I assume you will need to add a Best regards, Zibai |
@zibaiwan @pcolberg Ready for review now. This change preserves runtime backwards compatibility so it is safe to merge with older versions of the compiler. I believe that this needs to be merged before my compiler change goes into trunk. Also, with the new CSR version 5, forward compatibility is no longer possible (not sure if we support it in the first place). I can drop by tomorrow's runtime meeting to give a overview of the change if possible. Thanks :) |
sycl-l3 passed (fixed issues related to my change): https://spetc.intel.com/testsummary?testRunIds=7158950 |
@ericxu233, I took a quick look of your change, they look good to me. I will take a closer took after your overview tomorrow. Thanks Eric! |
@pcolberg @zibaiwan After further discussion with the compiler team, we decided to only go with the start signal change and not anything else for this release. So this is good to go in for now. The reason being is that the other signals may require more effort and redesign for the separation and not as important as the start signal. |
Thanks @ericxu233! Could you address the comment? Other than that, this is good to go 🙂 |
8b6f205
to
377a24e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Eric!
Enbaled backwards compatibility with the new CSR change
The compiler backend is now looking for support for CSR changes where we separate start into different registers rather than putting it into the CSR register. This will reduce kernel start latency since the runtime won't have to do a read before write and the CRA module would avoid a read operation which is not cheap at this moment (I think).