Repository navigation
[rocjitsu] DBI: Introduce mask policy for probes and pass EXEC as an argument - #11514
Conversation
kuhar
left a comment
There was a problem hiding this comment.
It would be good to update the stale policy documentation in this patch so it matches the implementation:
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/instrumentor.h:30still lists EXEC policy as future work, andinstrumentor.h:218-225still calls fields reserved and saysforce_full_execmust be false.emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/trampoline_builder.h:103-105says an EXEC-sourced argument uniquely forces the EXEC temp, although any argument opens the full-mask window, and line 111 says one toggle instead of the emitted two.emulation/rocjitsu/docs/dbi-design.md:80,:376, and:394still describe immediate-only arguments and a count-only deduplication key.
These are public contract/design comments, so aligning them here would prevent callers from following the previous milestone rules.
|
Made those suggested changes and updated the stale documentation |
4abe3ac to
ce449b0
Compare
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
kuhar
left a comment
There was a problem hiding this comment.
We should finish aligning the public documentation with the current probe contract. A few references still describe the previous behavior:
emulation/rocjitsu/docs/dbi-design.md:74says body reuse is keyed by(probe_obj, probe_symbol, argument count), but it is now keyed by object and symbol, with conflicting arity, source, or mask declarations rejected.emulation/rocjitsu/docs/dbi-design.md:193and:415list onlybuild_v_mov_b32_imm, although this patch adds and testsbuild_v_mov_b32_src.emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/instrumentor.h:218-224documents theforce_full_exec-without-probe check but omits the matchingprobe_argsrule.
Updating these remaining references together would give callers one consistent contract.
Ugh. I need to work on my prompting; my agents clearly have tunnel vision. Thanks for finding these. They are fixed. |
* Update ProbeKey to carry source of the argument * Added exec_lo and exec_hi as possible sources * Mask is read from saved EXEC value not from EXEC (which is -1 at the arg passing point in the envelope anyway) * Currently requires planner to reserve a temp register (it already needed to do this to save EXEC and we can get fancier later) * Rejects exec_hi for wave32 kernels
* Allow force_full_exec (except for inline nop) * Skip anchor mask restore if set and reset EXEC after * Set at instrumentation site, but ProbeKey has the policy too
+ cleanup and minor fixes
* Updated stale documentation * Use exhaustive switch for argument types with a runtime rejection to catch undefined argument types * Change probe key to use just name and symbol. At this point, there is no good reason to hold multiple copies of the same probe...the argument sources should not change and the EXEC policy definitely should not change. If these should change, then the probe should actually be different. * Added more tests
kuhar
left a comment
There was a problem hiding this comment.
It would be good to update emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/instrumentor.h:245 to name the actual remaining restriction. force_full_exec and probe_args are now consumed, so saying validate_anchor() rejects reserved InstrumentationPoint fields is stale; only non-zero filter_flags remains reserved.
LGTM
f274c61 to
1d537db
Compare
TheRock Submodule Bump ActivityNewest first
|
Motivation
This PR has two related parts:
Note: This requires PR #11332, which starts to pass arguments to probes.
Technical Details
force_full_exec(was there but not supported). The trampoline uses this information to decide whether to restore EXEC back to the anchor value before calling the probe (it gets set to -1 for spilling and setting arguments)Issue Tracking
Issue #8879
Test Plan
Note: This does not include tests for real hardware. These will be added when we can pass data to the host from the probe.
Test Result
Tests pass!
Submission Checklist