Skip to content

[rocjitsu] Fixing Bad Merge from EXEC Tracking PR - #9847

Merged
atgutier merged 2 commits into
developfrom
users/skushwah/fix-exec-registered-regen
Aug 7, 2026
Merged

atgutier merged 2 commits into
developfrom
users/skushwah/fix-exec-registered-regen

Conversation

@theSK2005

@theSK2005 theSK2005 commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

PR #7470 had a bad merge with PRs #8933 (intended to bring in register_exec_fn) and #9099 (intended to remove register_exec_fn) in _generator.py. Prior to PR #7470, develop didn't have this function and PR #7470 brought it back in when it shouldn't. Regeneration currently fails on develop, this fixes it. No changes to generated ISA.

Test Plan

Reran all rocjitsu tests

Test Result

All pass

Submission Checklist

PR #7470 had a bad merge with PRs #8933 and #9099 in _generator.py. Regeneration
currently fails on develop, this fixes it. No changes to generated ISA.
@theSK2005
theSK2005 requested review from a team and atgutier as code owners August 7, 2026 16:17
@therock-pr-bot

therock-pr-bot Bot commented Aug 7, 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/python/amdisa/codegen/_generator.py; 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

therock-pr-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

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

@atgutier atgutier 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.

Approving with comment:

[MINOR] /tmp/pr9847/head/emulation/rocjitsu/lib/python/amdisa/codegen/
_generator.py:8220 — The repaired ordinary split-instruction path lacks a
direct regression test

Reasoning: Removing the stale execute_registered_ =
register_exec_fn<...>() emission is correct, but no test exercises the
affected combination: ordinary instruction generation through gen_insts()
with split_execution_sources == true. Reintroducing the five deleted lines
would still pass the two tests currently asserting that
execute_registered_ is absent.

Evidence:

  • Gfx1250Profile is the only profile enabling split execution.

  • Ordinary constructors now obtain dense callback IDs through
    _split_execute_expr() and selected_exec_fn(id).

  • gen_execution_backend() builds the corresponding immutable callback
    table.

  • test_gfx1250_instruction_execution_backend_is_dense_and_scoped calls
    only gen_execution_backend().

  • test_gfx1250_vopd_template_uses_dx9_zero_and_fma exercises the separate
    bespoke VOPD generator.

  • No amdisa test calls gen_insts().

  • The exact-head amdisa suite nevertheless passes, demonstrating that the
    original regression path is currently outside test coverage.

Suggested fix: Add a focused Python test that generates one ordinary
gfx1250 instruction using gen_insts() or its smallest equivalent, then
verifies:

  • the model constructor contains selected_exec_fn(0);
  • the execution source contains the instruction’s execute_impl;
  • neither output contains execute_registered_ nor register_exec_fn;
  • optionally, the generated backend table places that instruction at the
    same dense index.

The test should fail on base e92aa6e… and pass on head bc71280….

Confidence: high.

@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 as long as the CI returns green

@newling

newling commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

The gcc job that is still failing might be fixed by: #9860

@atgutier
atgutier merged commit c4555b0 into develop Aug 7, 2026
23 of 24 checks passed
@atgutier
atgutier deleted the users/skushwah/fix-exec-registered-regen branch August 7, 2026 18:26
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