rocr: scope the VOP3PX trap PC fixup to gfx1250 revision 0 by shwetagkhatri · Pull Request #13797 · ROCm/rocm-systems

shwetagkhatri · 2026-10-20T03:48:12Z

Summary

The second-level trap handler's trap-during-VOP3PX PC fixup applies to gfx1250
revision 0 only. This scopes it to that revision instead of applying it to every
gfx1250 part.

JIRA ID : PLAT-212250

Changes

trap_handler_gfx12.s — the two VOP3PX encoding compares and the
.fixup_vop3px_middle PC-rewind block are guarded by a new assembler symbol
HAVE_VOP3PX_FIXUP. The symbol defaults to 1 under .ifndef, so a build that
does not override it produces the handler exactly as before. The S_SET_VGPR_MSB
fixup is deliberately left unguarded — it applies to every revision.

trap_handler/CMakeLists.txt — gfx1250 is built twice from the same source.
The build that keeps the fixup emits kCodeTrapHandlerV2_1250_rev0. The second
build passes -Wa,-defsym,HAVE_VOP3PX_FIXUP=0 and emits
kCodeTrapHandlerV2_1250, which serves revision 1 and later. The per-target
lists gain a parallel ASM_DEFSYM list so any future target can pass assembler
symbol overrides the same way.

amd_gpu_agent.cpp — GpuAgent::AssembleShader selects
compute_1250_rev0 when the agent's ISA minor version is 5, its
properties_.Capability.ui32.ASICRevision is 0, and that slot is populated.
Every later revision, and every later minor version, takes compute_1250 — the
blob assembled without the fixup. Shaders that do not vary by revision leave the
revision-0 slot zeroed and are served by compute_1250 for every revision.

Relationship to the kernel driver

This change stands on its own and does not depend on any kernel change landing.
It scopes the second-level trap handler only.

For context, a matching change to the KFD first-level CWSR handler has been
proposed but is not merged. In that proposal HAVE_INSTRUCTION_FIXUP splits
into HAVE_VOP3PX_FIXUP and HAVE_VGPR_MSB_FIXUP — both still keyed on
ASIC_FAMILY, not on revision — and the revision discrimination is done in
kfd_cwsr_init, which selects the revision-0 handler on adev->rev_id == 0.

In the currently merged kernel baseline there is no revision term at all:
HAVE_INSTRUCTION_FIXUP is gated on family alone, so the first-level handler
carries the VOP3PX fixup on every revision. That mismatch is the thing this
patch scopes on the ROCr side, and it is worth knowing when reviewing.

Effect on existing targets

The blob a revision 0 part receives is byte-identical to the pre-patch gfx1250
blob — md5 038cbb5a02520657c1bf1e14597af07b, measured against a purpose-built
pre-patch tree with a negative control.

That blob is the newly added symbol, so the byte-identity should not be read as
"nothing moved". The pre-existing symbol kCodeTrapHandlerV2_1250 now holds
different bytes (md5 138d64ed3657ce336d813aa98e2ff33f), because after this
change it is the blob assembled without the fixup.

No other target's output changes. HAVE_VOP3PX_FIXUP defaults to 1 under
.ifndef and the only entry that overrides it is the second gfx1250 build, so
every other target — including gfx1200, which assembles the same source file —
is built from source that is unchanged in effect. That is a source-level
argument; only the two gfx1250 blobs were measured.

🤖 Generated with Claude Code

therock-pr-bot · 2026-10-20T03:49:02Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	⚠️ Warning	Error: Source/code files changed without an accompanying unit test. Expected: add at least one test file named like `test_<name>.py` / `test_<name>.cpp` (or `<name>_test.`). Current:* code file(s) changed: `projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_gpu_agent.cpp`; no test file found
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

therock-pr-bot · 2026-10-20T03:49:04Z

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

Copilot

🔵 Needs a closer look

Revision-specific trap behavior and generated-blob equivalence need toolchain and hardware validation before approval.

No open issues 🎉

What changed in this PR

Scopes ROCr’s gfx1250 VOP3PX trap-PC fixup to revision 0 while preserving the S_SET_VGPR_MSB fixup for all revisions.

Changes:

Adds an assembler guard around the VOP3PX fixup.
Generates a separate gfx1250 handler for later revisions.
Selects the handler using the ASIC revision, with fallback to the existing blob.

File	Description
projects/rocr-runtime/runtime/hsa-runtime/core/runtime/trap_handler/trap_handler_gfx12.s	Conditionally assembles the VOP3PX fixup.
projects/rocr-runtime/runtime/hsa-runtime/core/runtime/trap_handler/CMakeLists.txt	Builds the later-revision handler using a symbol override.
projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_gpu_agent.cpp	Selects the revision-specific handler.

_{Review effort: Balanced}

The second-level trap handler's VOP3PX instruction-boundary PC fixup applies to gfx1250 revision 0 only. Guard it behind an assembler symbol so it is compiled out for later revisions, emit a second trap handler without it, and select between them from the ASIC revision the kernel reports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Name the revision-specific handler slot after the revision it serves, and narrow the revision check so later minor versions are unaffected. Default the assembler symbol off so the revision 0 build requests the PC rewind explicitly rather than every other build switching it off. A target added to the build lists without a matching assembler-symbol entry then assembles without the rewind, which is correct for every revision except revision 0. Catch an incomplete per-revision shader table entry instead of silently falling back, and fail configure early if the parallel target lists fall out of step. Keep the masking instruction with the compares it feeds so it is not emitted when those compares are not, and drop a stale comment reference to the first-level handler. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

cfreeamd

🤖 AI-assisted pre-review (for @cfreeamd)

[Required] PR title format: rocr: scope the VOP3PX trap PC fixup to gfx1250 revision 0 is missing the type(component): prefix. ROCm squash-merges PRs, so the title becomes the permanent commit message. Suggested: fix(rocr): scope the VOP3PX trap PC fixup to gfx1250 revision 0.

All code findings from earlier review passes have been addressed (assert added for the rev0.code == NULL fallback; macro direction corrected to opt-in). This title correction is the only remaining item.

shwetagkhatri requested review from atgutier, cfreeamd, dayatsin-amd, kentrussell and lancesix as code owners October 20, 2026 03:48

Copilot AI balanced review requested due to automatic review settings October 20, 2026 03:48

github-actions Bot added the project: rocr-runtime label Oct 20, 2026

therock-pr-bot Bot added the Not ready to Review label Oct 20, 2026

Copilot started reviewing on behalf of shwetagkhatri October 20, 2026 03:49

View session

Copilot AI reviewed Oct 20, 2026

View reviewed changes

shwetagkhatri requested a review from jcornwallAMD October 20, 2026 03:51

systems-assistant Bot added the organization: ROCm label Oct 20, 2026

dayatsin-amd reviewed Oct 20, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_gpu_agent.cpp

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_gpu_agent.cpp Outdated

shwetagkhatri force-pushed the users/shwetak/PLAT-212250_gfx1250-rev1-trap-handler branch from 05ef925 to 67b3f8b Compare October 20, 2026 17:40

shwetagkhatri requested a review from dayatsin-amd October 20, 2026 18:08

lancesix reviewed Oct 20, 2026

View reviewed changes

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/trap_handler/CMakeLists.txt Outdated

Comment thread projects/rocr-runtime/runtime/hsa-runtime/core/runtime/trap_handler/trap_handler_gfx12.s

shwetagkhatri and others added 2 commits October 21, 2026 16:13

shwetagkhatri force-pushed the users/shwetak/PLAT-212250_gfx1250-rev1-trap-handler branch from 67b3f8b to 8772c6c Compare October 21, 2026 20:35

shwetagkhatri requested a review from lancesix October 21, 2026 22:18

shwetagkhatri removed the Not ready to Review label Oct 22, 2026

cfreeamd reviewed Oct 22, 2026

View reviewed changes

Navigation Menu

Repository navigation

rocr: scope the VOP3PX trap PC fixup to gfx1250 revision 0 - #13797

rocr: scope the VOP3PX trap PC fixup to gfx1250 revision 0#13797
shwetagkhatri wants to merge 2 commits into
developfrom
users/shwetak/PLAT-212250_gfx1250-rev1-trap-handler

shwetagkhatri commented Oct 20, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Oct 20, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Oct 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfreeamd left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

shwetagkhatri commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Relationship to the kernel driver

Effect on existing targets

Uh oh!

therock-pr-bot Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Policy Checks Passed

Uh oh!

therock-pr-bot Bot commented Oct 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfreeamd left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shwetagkhatri commented Oct 20, 2026 •

edited

Loading

therock-pr-bot Bot commented Oct 20, 2026 •

edited

Loading