Repository navigation
[rocjitsu] Fix operand extensions, LDS bounds and fork targets - #14434
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
rocjitsu presubmit fuzz passedThe 32-worker multiplexer campaigns for the merge result for PR head
|
newling
left a comment
There was a problem hiding this comment.
Looks fine to me, an agent review found 3 things. One has a suggested fix (commit), one might be better left for a follow-up, one is an issue that predates this PR. PTAL: https://github.com/newling/agent-public-reviews/blob/main/rocm-systems/review_pr14434.md
85d45c3 to
5b24fd8
Compare
atgutier
left a comment
There was a problem hiding this comment.
LGTM with one style nit.
atgutier
left a comment
There was a problem hiding this comment.
MINOR
[MINOR] lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/scalar_operand_resolve.h:151 — resolve_delegated_inline_float16 never
fires in production
Reasoning: It returns a value only when the bottom of the delegate chain has
has_register_selector() == true. Every delegate that production code binds is a
StagedOperand, which keeps the base default (false, operand.h:192). So outside
tests it always returns nullopt. It still adds a chain walk and a virtual call
to every 16-bit DPP/SDWA delegated read, once per lane in read_lane_exec. The
PR text says lane, SIMD and delegated reads agree, but no production path ever
produces that delegated state.
Evidence: Delegate bindings are std::optional dpp_src0_/dpp_src1_
(_generator.py:13444,13450, 3,775 generated instances) and rel_staged_src
(vector_special.py:271,305). No other ScopedOperandDelegate producer exists.
The call sites are isa_operand_simd_inl.h:64 and the generated
read_lane_exec/read_lane_chunk_exec in all ten operand_exec.cpp files. Only
vector16_inline_constant_test.cpp (~259-273, ~427-438) builds a delegate that
still holds a selector.
Suggested fix: Remove the helper, its template and generated call sites, and the
two fresh-delegate test blocks; keep the staged-delegate assertions. Re-add it
together with a real producer of non-staged delegates if one is planned.
Confidence: High on reachability (every producer enumerated); impact is small.
[MINOR] docs/plugins.md:498, docs/race-detector.md:230 — docs not updated for the new LDS behavior
Reasoning: plugins.md still says "RDNA4 FLAT low-17-bit address truncation
remains a separate follow-up", but this PR implements it
(compute_unit.cpp:1012-1029). Neither doc mentions the new public observation
fields flat_local_oob_lane_mask / flat_local_allocation_end
(memory_access_observation.h). race-detector.md:230 says LDS intervals are
bounded only by the CU's LDS backing, but RDNA4 FLAT now also clips at the
workgroup's allocation end. An observer author would not learn that whole lanes
can be rejected, or kept only up to the last whole element, while
valid_lane_mask and request_lane_mask stay unchanged.
Suggested fix: Replace the follow-up sentence with the rule:
- the offset wraps to 17 bits before the workgroup base is added;
- a lane is rejected if its first element doesn't fit;
- other lanes keep only the whole elements below the allocation end;
- atomics need their complete element;
- masks and counters are unchanged.
Add one matching sentence at race-detector.md:230.
Confidence: High.
NIT
[NIT] memory_pipeline.cpp:1246-1250, race_detector/plugin.cpp:392-396, race_detector/core/wave_race_state.cpp:23-29 — whole-element
clipping written three times
Reasoning: Each copy computes available = addr < end ? end - addr : 0; bytes = min(bytes, available / elem * elem). plugin.cpp's validate clips, then
registerLdsEvent(..., flat_local_allocation_end) clips the same lanes again.
The copies agree today. If the execution rule changes later, the race
detector's intervals could quietly diverge from execution.
Suggested fix: Add one inline helper next to the observation type and call it
from all three sites.
Confidence: High.
[NIT] lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/scalar_f16_source.h:19-24 — duplicated "effective inline selector" check
Reasoning: read_scalar_f16_carrier repeats the delegate walk and the
reads_value() && has_register_selector() test from
scalar_operand_resolve.h:154-158. Fixing the first MINOR mostly removes this;
otherwise share one predicate.
Confidence: High.
5b24fd8 to
d24391a
Compare
sjain-stanford
left a comment
There was a problem hiding this comment.
It would be good to apply the project's type and naming conventions to these new definitions (paths below are under emulation/rocjitsu/):
tests/dbt/translate_gfx1250_test.cpp:6192: makeRestoreScalarGateaclasswith a public destructor and privateprevious_state. It owns restoration behavior; the Types rule applies, andForceScalarGuardintests/cdna5_instruction_test.cpp:63provides an example.tests/scalar_f16_constant_test.cpp:163andtests/vector16_inline_constant_test.cpp:184: suffix the five protected fixture members with_, as the Naming rule requires. This distinguishes persistent fixture state from local setup variables.lib/rocjitsu/src/rocjitsu/isa/operand.h:233: spell the single-precision enumeratorF32, matching neighboringSourceModifierFormat,ResultFormatandfp_format::F32. Update the generator strings, expectations and generated outputs together so the same format has one name across these interfaces.
We should update the observer guides and field comment to describe the implemented bounds, including the modeled allocation limit. Following the existing observer-documentation feedback, the remaining locations under emulation/rocjitsu/ are:
docs/plugins.md:498: replace the claim that RDNA4 FLAT truncation is future work. The offset wraps to 17 bits before adding the workgroup base; a lane whose first element does not fit is rejected, other transfers retain complete elements, and atomics require their whole element. Explainflat_local_oob_lane_maskandflat_local_allocation_endwhile retaining nominal masks and counter participation.docs/race-detector.md:230: say intervals also stop at the captured workgroup allocation end, rather than describing only the CU backing limit.lib/rocjitsu/src/rocjitsu/vm/plugins/memory_access_observation.h:214: carry through the limitation already documented atcompute_unit.cpp:1012: this islds_base + lds_size, including current 256-byte scheduler rounding; target allocation and encoding granules remain unmodeled. PM4 sizes may already be 512-byte multiples before that rounding.
This keeps observer authors from treating nominal requests as accessed bytes or mistaking the modeled end for a fully qualified hardware allocation boundary.
d24391a to
b819930
Compare
newling
left a comment
There was a problem hiding this comment.
LGTM. Nothing new in latest review
sjain-stanford
left a comment
There was a problem hiding this comment.
This would be easier to maintain if _pc_target_body() emitted its required kPcAddressMask declaration, with the same declare_mask option as _pc_export_body(). In emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py:6616, it references that name without declaring it, so SETPC, indirect fork and SWAPPC each repeat the mask setup at lines 7365, 7390 and 7409.
Only SWAPPC needs to share a declaration between both helpers; it can disable declaration in one of them. Moving the setup into the helper removes that hidden prerequisite from the other callers while preserving the generated code.
The extension audit following triton-lang/triton#12512 exposed source-format, destination, LDS addressing and indirect-PC defects. - Format numeric scalar F16 inline sources as half values, including HI conversion, while preserving full register/literal carriers and integer SEXT/PACK patterns. Use one producer policy for HI carriers. Pseudo-scalar F16 transcendental inlines retain their raw FP32 carriers in both decoded operand metadata and scalar execution. - Record the F16, FP32 or BF16 format of 16-bit VALU inline floats at construction. Lane, SIMD and staged delegate reads agree. CDNA5 BF16 keeps an FP32 carrier for high-half selection; earlier BF16 sources use their low numeric pattern. Raw scalar/constant APIs retain their carrier contract. Remove the unused SIMD broadcast path and unreachable delegate hook. Preserve CDNA4 numeric BF16 inline values in CDNA3 translation; use exact FP32 moves and a widened BF16 literal for inverse-two-pi. Replicate integer and floating true16 DOT2 accumulators before OPSEL, while keeping register/literal half selection unchanged. - Preserve RDNA1/2 upper destination bits for ordinary 16-bit ALU results through an explicit partial-write property. Keep packed saturation's full-word zero padding and the existing scalar, memory, packed, MIX and SDWA destination policies. - Apply RDNA4 unsigned 17-bit LDS offset math and clip multiword loads/stores to complete in-range elements within the current modeled workgroup allocation. Atomics require their complete element. Capture the allocation end across retries and publish rejection/bounds facts to observers. Race intervals retain only accessed bytes, without changing nominal requests or counters. - Share SETPC/SWAPPC target decoding with CDNA1-4 indirect forks. Keep sign-filled GETPC values as raw 48-bit internal targets, preserve positive linked-rel32 targets, and leave fall-through and branch-stack PC assignment with the caller. Reuse the existing physical-memory adapter in page-table PC fetch tests. gfx1100 and gfx1201 qualify the VALU integer and BF16 inline tables. LDS boundary probes on gfx1201 confirm complete-element clipping, including byte-unaligned FLAT accesses; the DS byte-store rule does not describe this FLAT path. RDNA1/2 destination preservation has manual/LLVM coverage without physical qualification. The manuals' low-half-only F16 inline wording predicts zero for HI conversion, while gfx1201 supplies the numeric half value. CDNA5 and RDNA3.5 follow that measurement by extrapolation. CDNA4/5 BF16 rules have source-backed coverage; no physical result is claimed there. CDNA5 VOP1 keeps the raw low-half interpretation described by LLVM; cover both VOP3 halves explicitly while the manual passages remain inconsistent. Pseudo-scalar carrier behavior matches 110 gfx1201 source/mode checks and is extrapolated to CDNA5. RDNA4 inline decoding remains a separate gap. Keep unsupported instruction families, legacy RDNA1/2 integer and F16 ternary OPSEL support, aperture classification before IOFFSET, and target-specific LDS allocation/encoding granules with DS bounds as separate follow-up work. The two legacy OPSEL tests skip explicitly. The operand/LDS work matches 224 hardware/model cases using identical GPU binaries. PC regressions use source-backed simulation coverage. Pass focused CTests, generator tests and GCC syntax checks for the changed test translation units. DOT2 plain and DPP probes match gfx1100/gfx1201 in scalar/SIMD execution. The eight added CDNA1-4 fork regression groups fail before the fix. They cover zero/nonzero load bias, uniform and divergent paths, join resume addresses, and positive linked-rel32 targets. Execute CDNA4 BF16 conversions and their CDNA3 translations against independent bit-pattern oracles, covering all nine floating selectors, raw register/integer-inline controls, scalar/SIMD execution and empty/sparse/full EXEC masks. All floating selectors fail before the translation correction.
b819930 to
02e5f36
Compare
rocjitsu post-commit fuzz findingThe 32-worker multiplexer campaigns for commit
A retained input can expose a pre-existing defect; this result does not by itself show that the PR introduced it. Download the saved inputs and AFL++ statistics (retained for 14 days). |
Six retained TSan fuzz inputs abort when a mutated gfx942 stream reaches DS_GWS_BARRIER before the standalone execution harness has initialized its process-scoped GWS resource. This is a deterministic harness-state failure, not a TSan race. Arm all 64 resources for process 1 with count zero before execution. Document the contract and cover representative resource IDs with an execution-count regression test. All six inputs pass under TSan, as do 22 focused ExecutionFuzz and MultiplexerFuzz tests. Two local Peanut review rounds completed. Identified in #14434 (comment)
TheRock Submodule Bump ActivityNewest first
|
The extension audit following triton-lang/triton#12512 exposed source-format, destination, LDS addressing and indirect-PC defects.
lds_base + lds_sizeend, including current 256-byte scheduler rounding; target allocation and encoding granules remain a follow-up.gfx1100 and gfx1201 qualify the VALU integer and BF16 inline tables. LDS boundary probes on gfx1201 confirm complete-element clipping, including byte-unaligned FLAT accesses; the DS byte-store rule does not describe this FLAT path. RDNA1/2 low-half destination preservation has manual/LLVM coverage without physical qualification. High-half OPSEL for legacy integer and F16 ternary instructions, including V_FMA_F16 and V_MIN3/MAX3/MED3_F16, remains a follow-up for both source and destination selection.
The manuals' low-half-only F16 inline wording predicts zero for HI conversion, while gfx1201 supplies the numeric half value. CDNA5 and RDNA3.5 follow that measurement by extrapolation. CDNA4/5 BF16 rules have source-backed coverage; no physical result is claimed there. CDNA5 VOP1 keeps LLVM’s raw low-half interpretation where the manual passages conflict; VOP1 and both VOP3 halves have explicit execution coverage, including inverse-two-pi. Pseudo-scalar carrier behavior matches 110 gfx1201 source/mode checks and is extrapolated to CDNA5. RDNA4 inline decoding remains a separate gap.
Keep unsupported instruction families, legacy RDNA1/2 OPSEL support for integer and F16 ternary forms, aperture classification before IOFFSET, and target-specific LDS allocation/encoding granules with DS bounds as separate follow-up work. The two legacy OPSEL tests skip explicitly.
The operand/LDS work matches 224 hardware/model cases using identical GPU binaries. PC regressions use source-backed simulation coverage. Pass 1,090 focused CTests (29 skipped), 2,898 generator tests (46 skipped), and GCC syntax checks for all three edited test translation units. Canonical generation checks all 705 files. ISA profile and observer documentation reflect the implemented source policies and current modeled bounds. DOT2 plain and DPP probes match gfx1100/gfx1201 in scalar/SIMD execution.
The eight added CDNA1-4 fork regression groups fail before the fix. They cover zero/nonzero load bias, uniform and divergent paths, join resume addresses, and positive linked-rel32 targets.
Execute CDNA4 BF16 conversions and their CDNA3 translations against independent bit-pattern oracles, covering all nine floating selectors, raw register/integer-inline controls, scalar/SIMD execution and empty/sparse/full EXEC masks. All floating selectors fail before the translation correction.