Skip to content

[rocjitsu] Assert the instrumentor architecture invariant - #14404

Merged
IanWood1 merged 2 commits into
developfrom
users/IanWood1/rocjitsu-instrumentor-arch-errors
Oct 28, 2026
Merged

IanWood1 merged 2 commits into
developfrom
users/IanWood1/rocjitsu-instrumentor-arch-errors

Conversation

@IanWood1

@IanWood1 IanWood1 commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

Motivation

Remove the unreachable architecture exception from the instrumentor’s VGPR allocation helper.

Technical Details

Replace the throw with an assertion after an exhaustive architecture switch. Explicitly handle unsupported enum values so adding an architecture requires an allocation-model decision through compiler switch diagnostics. Patch admission still rejects unsupported inputs through the existing result.

Existing instrumentor, spill, and target-registry tests pass. No new tests are added; runtime assertion firing is not tested in the RelWithDebInfo build.

Issue Tracking

Part of #10239.

Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
@therock-pr-bot

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

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

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: emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/patch/instrumentor.cpp; no test file found
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

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

🙋 Wish to Override Policy?

@therock-pr-bot

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@IanWood1
IanWood1 marked this pull request as ready for review October 28, 2026 17:44
Copilot AI balanced review requested due to automatic review settings October 28, 2026 17:44
@IanWood1
IanWood1 requested review from a team and atgutier as code owners October 28, 2026 17:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@accauble accauble left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one note to consider

Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@IanWood1
IanWood1 merged commit afeccc6 into develop Oct 28, 2026
67 of 68 checks passed
@IanWood1
IanWood1 deleted the users/IanWood1/rocjitsu-instrumentor-arch-errors branch October 28, 2026 20:12
@github-actions

Copy link
Copy Markdown
Contributor

rocjitsu post-commit fuzz passed

The 32-worker multiplexer campaigns for commit afeccc67b92a completed without recording a crash or hang.

  • ASan+UBSan (6-minute): 600,898 input executions (1,703.73 inputs/s)
  • TSan (6-minute): 541,097 input executions (1,545.00 inputs/s)

View the workflow run.

@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants