Repository navigation
[rocjitsu] Return scalar builder failures as results - #14536
Conversation
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
✅ 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. |
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
Can we make the migrated test instruction sequences easier to read by reusing repeated words and keeping the builder call close to its position in the sequence? In emulation/rocjitsu/tests/dbt/translate_gfx1250_test.cpp:6949, six identical NOPs are built and checked separately as maybe_s_nop_27 through _32; one checked nop would suffice. The numbered locals in tests/analysis/liveness_test.cpp:4402 and tests/dbi/scalar_builders_test.cpp:27 similarly make readers trace suffixes back to operands. Use scope-local descriptive names, and consider a shared checked append helper (as in tests/dbi/log_record_probe.h) for the longer sequences. Preserve the success checks and focused rejection tests.
I think we should forward the builder diagnostics through append_nop_padding and append_direct_branch_island_pool, then reuse them at their callers instead of copying instruction-specific error strings. Both helpers discard the diagnostic today (emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/kernel_text_layout.cpp:94 and :662); the pool caller in code/dbt/binary_translator.cpp:4434 labels any failure as s_nop, although the helper also builds branches. code/patch/instrumentor.cpp:705 repeats the same message instead of passing an emitter. Current architecture checks prevent the translator's late branch-failure case, so this is a diagnostic-maintenance concern, not a demonstrated translation failure. Passing the optional emitter through these helpers would keep the reason with the builder without requiring a public patcher API redesign.
We should reject SOPP opcodes above 0x7f in emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/builders/instruction_builder.h:125, alongside the other scalar field checks. build_sopp_encoding(arch, 0x80, 0) currently succeeds and produces exactly the word for opcode zero on all ten AMDGPU architectures; the generated builders mask the opcode to seven bits. This omission predates the PR, but it leaves the newly documented invalid-field contract incomplete. Add the range check and cover 0x7f/0x80, including the failure diagnostic, in ScalarLayoutsRejectEveryOversizedFieldBeforeNarrowing.
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
I think we should finish forwarding the builder diagnostics through the remaining relocation and instrumentor checks. The padding/island-pool helpers now forward them, but emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/kernel_text_layout.cpp:867, :974, :1028, and :1111 still copy the NOP/branch error strings, as do code/patch/instrumentor.cpp:416 and :881. Use the existing OptionalStringDiagnosticSink adapter and pass its emitter to the builder, retaining the relocation context where applicable. The copied messages match today; the benefit is keeping the failure reason in one place, as append_skipped_kernel_stub already does.
Can we apply the test-readability cleanup to the remaining migrated sequences too? In emulation/rocjitsu/tests/patch/trampoline_builder_test.cpp:58 and :77, the same NOP is still built twice as maybe_s_nop_5 and maybe_s_nop; reuse one checked nop, and name the distinct branches forward_branch and return_branch instead of numbered locals. The longer sequence in tests/analysis/reachable_cfg_test.cpp:562 still separates each builder from its insertion with a temporary/assert/unwrap block. The new append_checked_words helper can keep those builder calls together in instruction order while preserving the success checks.
Preserve builder reasons and probe-call stage context at remaining callers. Simplify migrated test sequences while retaining checked setup. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Keep the builder success checks while using the encoded word directly in repeated setup and expectations. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
It would help to forward the optional diagnostic emitter through append_pc_delta_builder (emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/builders/instruction_builder.h:957) and its private add/sub helpers. They currently discard the new SOP2 field-failure reason, so the relocation callers at code/patch/kernel_text_layout.cpp:1207 and :1221 can only report that the target ISA cannot encode the builder.
Pass the builder reason through the existing string sink while retaining the relocation offset/context. Current recovery and architecture checks constrain these operands, so this is a diagnostic-maintenance improvement, not a demonstrated translation failure; the existing boolean return contract can remain.
Reject opcode values that change instruction families and preserve architecture validation diagnostics. Forward builder reasons with stage context and cover the failure paths with focused regressions. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Retain checked scalar encodings while adopting fallible topology setup. Adapt new upstream VM test kernels to check builder results before use. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
It would be useful to document the two contracts added here so callers can rely on them without reading the implementations:
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/error_report.h:33: the optional prefix is owned by the sink and prepended on every replacement of the output string; it does not accumulate across messages.emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/builders/instruction_builder.h:957:append_pc_delta_builderreturns true after appending the sequence, or false without changingwords; a supplied emitter receives at most one failure diagnostic.
The prefix-ownership and PC-delta failure tests already exercise these guarantees. A short constructor/class description and an @returns entry would expose them alongside the existing lifetime documentation.
I think we should forward the existing emitter through the remaining sequence helpers in emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/kernel_text_layout.cpp:363 and :830, including their shared append_long_pc_transfer helper at :447. They still call scalar builders and append_pc_delta_builder without it, so the callers at :985 and :1075 replace a builder failure with a generic sequence message despite already owning diagnostic sinks.
The recovered-builder path at :1210 now preserves the specific reason; these sibling paths should retain it with their relocation context too. Keep fallback messages for non-builder failures such as an unrepresentable offset. Current architecture and register checks constrain these paths, so this is a diagnostic-maintenance request, not a demonstrated translation failure.
Forward the borrowed emitter through recovered and long branch sequences so relocation failures retain the builder reason and source offset. Document the append and sink contracts and test unsupported PC-delta diagnostics. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
I think we should finish preserving the builder reason and relocation stage in emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/kernel_text_layout.cpp:
- At
:794,append_branch_island_direct_sequencestill drops diagnostics frombuild_s_branchandbuild_inverted_conditional_skip; its caller substitutes a generic island-sequence error. Pass the existing emitter through this helper and make the pointer explicit at both callers ofbuild_inverted_conditional_skip(:703). Preserve the window-growth handling and fallback for failures without a builder message. - At
:988and:1080, the long-direct and recovered-indirect paths now forward the reason but lose the stage. Prefix it withdirect branch long sequence:orcanonical recovered indirect branch sequence:, as the recovered-builder path already does. For example, a long branch withlong_branch_sgpr = 128reports onlys_getpc_b64 encoding field out of range, although the source instruction is a branch. UpdateRecoveredIndirectSequenceRetainsBuilderDiagnosticand add a direct-branch diagnostic case, retaining the generic fallback when no builder text is emitted.
Current architecture and register-allocation checks constrain these failures; this is about maintaining the diagnostic contract, not a demonstrated translation failure.
Forward island builder diagnostics and retain direct and recovered branch stages when a checked builder rejects an encoding. Cover the long-branch failure without publishing partial text. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
Left an inline reply and some new comments
Forward padding diagnostics through control-flow growth and retain stage context at translation. Reuse the spill diagnostic sink and expose branch-immediate optional types. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
It would help to pass the expected diagnostic into the test helper at emulation/rocjitsu/tests/dbt/translate_gfx1250_test.cpp:3521. Expect Unimplemented instruction: s_nop for target architecture for the unsupported architecture and an empty string for malformed requests. Substituting could not pad control-flow window for the builder reason still passes the current assertion. The parameter comment at emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/kernel_text_layout.h:282 could state that the borrowed emitter receives padding failures and stays silent for invalid requests.
LGTM
Document the borrowed diagnostic emitter lifetime and use explicit optional types. Check the exact unsupported-architecture diagnostic in the tests. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
The new sibling-branch regression test used scalar builder results directly in an integer vector. Check and unwrap those results before constructing the instructions so the merged branch builds with the fallible API. Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
rocjitsu post-commit fuzz passedThe 32-worker multiplexer campaigns for commit
|
TheRock Submodule Bump ActivityNewest first
|
## Motivation Make DBI test fixtures report encoding failures explicitly so rejected inputs cannot be consumed as successful instruction encodings. ## Technical Details Return Result or FailureOr from global-memory builders and log-record probe helpers, preserve their rejection diagnostics, and check results in simulator fixtures. Invalid offsets, register tuples, architectures, slot counts, and oversized probe sites have regression coverage. The RelWithDebInfo wrapper build, changed-file hooks, and 295 focused builder, probe, spill, and DBI tests passed. HSA simulation tests required running outside the sandbox after topology and memory-keeper permission failures. Full-suite, corpus, sanitizer, and hardware validation were not run. ## Issue Tracking Part of #10239. Builds on the merged scalar-builder changes in #14536. --------- Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Motivation
Remove explicit exceptions from scalar instruction builders and operand-code lookup.
Technical Details
Return failures with diagnostics for unsupported architectures and invalid scalar encoding fields. Propagate results through trampoline planning, patch layout, instrumentation, and translation before publishing dependent output. Preserve builder reasons with relocation and control-flow growth context, keeping generic fallbacks when no diagnostic is emitted.
Update affected tests to check builder results. Focused regressions cover rejected inputs and unchanged output on failure.
Issue Tracking
Part of #10239.