-
Notifications
You must be signed in to change notification settings - Fork 571
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
i#5121: Add test for getting app value in multi-phase drreg #5126
base: master
Are you sure you want to change the base?
Conversation
Adds a test that demonstrates how to get the app value for a reg that was reserved in multiple phases using drreg. This is complex as in the current design a phase doesn't know about spill done in earlier phases. So, getting the app value in insertion phase for a reg that was reserved in app2app and insertion phases both requires both the phases to work together. Adds to the documentation of drreg_get_app_value and drreg_statelessly_restore_app_value that they allow obtaining the reg's meta value from the last phase where it was reserved, OR the app value if there's no such previous phase. Issue: #5121
I think it should be possible to avoid this multi-step complexity at the client side. In the insertion phase, we can find the spill slot used in the app2app phase for the actual app value. It should be the first spill after the last app write to the reg (or start of bb). If this spill was added by insertion phase, then there's no overlapping spill regions, otherwise we restore from that earlier spill slot. The implementation might be tricky though, as we're restoring from a slot that the current phase doesn't know contains anything. What would stateful restoration mean in this case? Because the insertion phase needs to preserve the app2app value of the reg too. |
Actually, using the first spill slot after the last app write won't work if that spill slot was later recycled. I remember this being a corner case also in the drreg state restoration code. I guess we can invoke the state restoration code itself in some form to figure out the correct spill slot for the to-be-restored reg; only when the BB has the multi phase use flag set. |
@derekbruening I thought it'd be useful to add an example for how the app2app phase should restore a spilled reg value so that the insertion phase can see it. |
This is still marked as a draft.
If the idea is for users to see how to write proper app2app changes, should it be in api/samples/ then instead of subtest number 39 in a large drreg test? |
@@ -459,6 +459,11 @@ DR_EXPORT | |||
* \p *swap. It is up to the caller to un-reserve the register in | |||
* that case. | |||
* | |||
* For regs that were reserved in a prior phase also, this routine | |||
* restores the reg's meta value from that prior phase, OR if the |
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.
This doesn't seem good enough: users don't know what registers the scatter/gather expansion used, and so users can't write correct clients. It seems like we have to not overlap scratch registers between app2app and insertion or something, or have the scatter expansion provide guarantees on its behavior so a client knows where it can get app values.
A real-world case to focus on is #3995 where one method of implementation is phases of instrumentation implemented with flushes. With flushes invoked from clean calls, the call has to get the proper app state. Maybe having just one point at the last instruction of the basic block where the state is available is good enough there.
@abhinav92003 -- trying to clean up stil-open PR's: is still still valid? |
Adds a test that demonstrates how to get the app value for a reg
that was reserved in multiple phases using drreg. This is complex
as in the current design a phase doesn't know about spills done
in earlier phases. So, getting the app value in insertion phase for
a reg that was reserved in app2app and insertion phases both
requires both the phases to work together.
Adds to the documentation of drreg_get_app_value and
drreg_statelessly_restore_app_value that they allow obtaining
the reg's meta value from the last phase where it was reserved,
OR the app value if there's no such previous phase.
Issue: #5121