[theSK2005]

Exposed flags to label instructions that ignore EXEC or write to EXEC so it is accessible by liveness. Then iterate EXEC state per instruction in reverse post order so that liveness VGPR/accVGPR kills can be done properly. Moved some resolve_scalar functions into a shared header to reduce code overlap.

Defer handling instructions that require EXEC to be AllOnes.

Addresses Issue #7025.

[accauble]

This is my first pass of the codegen. Haven't reviewed the analysis yet. Mostly nit-picky things and a couple questions I have that could improve the code if it is feasible.

[atgutier]

This overall LGTM.

An agent review flagged a few things that would be good to address:

MINOR (recommended before merge)

  1. exec_state.cpp:131-161 — loop-header-as-scope-entry seeding edge case (soundness-adjacent). A
  block that is both the kernel-scope entry and the target of an in-scope back-edge (loop header at
  offset 0) is classified non-entry and seeded optimistically Full instead of Unknown — the only path
  by which the forward MUST analysis could compute Full where it should be Unknown (→ an unsound
  kill). In practice the absolute first block of a kernel cannot have an in-scope back-edge, so it's
  robustness, not an observed bug. Fix: treat a block as entry-for-seeding if it's the scope leader OR
   has no in-scope predecessor.
  2. RESULT_COPY/RESULT_OR derived by string-matching semantic_class/operation while
  WRITES_EXEC/IGNORES_EXEC come from the AST. A future EXEC-writing copy/or opcode under a class not
  in the hand-list silently loses the combinator flag (degrades safe, but latent drift). Fix: derive
  the combinator from the same AST path so the two flag sets can't diverge.

  NIT

  - exec_state.h:7-13 file-level @details ends mid-sentence ("two-point lattice:") without enumerating
   Full/Unknown.
  - Internal enums (ExecWrite, Combinator) lack explicit : uint8_t underlying type while public
  ExecState has it.
  - binary_translator.cpp:567 benign uint8_t→uint32_t wave-size widening; an assert(wave_size == 32 ||
   64) in the ExecMaskAnalysis ctor would document the invariant.
  - s_cbranch_execz/execnz get IGNORES_EXEC (correct — branches aren't lane-masked) but no READS_EXEC.
   These branches do test EXEC; whether that should mark EXEC live for liveness is a semantics call
  worth deciding. If yes, it's a generator change (emit READS_EXEC for those opcodes).

[accauble]

Overall I think this is great. There is still some clean up required. I like the refactor that moves code to the scalar_operand_resolve header but I think @kuhar should weigh in on it once the tests all pass.

[theSK2005]

This overall LGTM.

An agent review flagged a few things that would be good to address:

MINOR (recommended before merge)

  1. exec_state.cpp:131-161 — loop-header-as-scope-entry seeding edge case (soundness-adjacent). A
  block that is both the kernel-scope entry and the target of an in-scope back-edge (loop header at
  offset 0) is classified non-entry and seeded optimistically Full instead of Unknown — the only path
  by which the forward MUST analysis could compute Full where it should be Unknown (→ an unsound
  kill). In practice the absolute first block of a kernel cannot have an in-scope back-edge, so it's
  robustness, not an observed bug. Fix: treat a block as entry-for-seeding if it's the scope leader OR
   has no in-scope predecessor.
  2. RESULT_COPY/RESULT_OR derived by string-matching semantic_class/operation while
  WRITES_EXEC/IGNORES_EXEC come from the AST. A future EXEC-writing copy/or opcode under a class not
  in the hand-list silently loses the combinator flag (degrades safe, but latent drift). Fix: derive
  the combinator from the same AST path so the two flag sets can't diverge.

  NIT

  - exec_state.h:7-13 file-level @details ends mid-sentence ("two-point lattice:") without enumerating
   Full/Unknown.
  - Internal enums (ExecWrite, Combinator) lack explicit : uint8_t underlying type while public
  ExecState has it.
  - binary_translator.cpp:567 benign uint8_t→uint32_t wave-size widening; an assert(wave_size == 32 ||
   64) in the ExecMaskAnalysis ctor would document the invariant.
  - s_cbranch_execz/execnz get IGNORES_EXEC (correct — branches aren't lane-masked) but no READS_EXEC.
   These branches do test EXEC; whether that should mark EXEC live for liveness is a semantics call
  worth deciding. If yes, it's a generator change (emit READS_EXEC for those opcodes).

Fixed the all the issues except comment #2. I think moving the RESULT_COPY and RESULT_R flags into the AST could be a follow-up PR.

[therock-pr-bot]

✅ All Checks Passed — Ready for Review

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🔎 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]

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

[therock-pr-bot]

Pre-commit check failed

⛔ pre-commit failed

Please run locally:

• python -m pip install pre-commit
• pre-commit install
• pre-commit run --all-files --show-diff-on-failure

This repo uses .pre-commit-config.yaml.

[accauble]

Still need to update a default wave size in exec_state.h.

[kuhar]

A few comments

[accauble]

@theSK2005 could you rebase please?

[kuhar]

A couple of comments, the other one is above: #7470 (comment)

[accauble]

LGTM - I would say that tracking the must be all ones is a good follow-up and not required for this PR.

[kuhar]

This seems to be ~10% slower on rccl hotswap translation -- we need to bring the overhead down before this can land.

[accauble]

This seems to be ~10% slower on rccl hotswap translation -- we need to bring the overhead down before this can land.

Does the rccl translation need the EXEC state tracking for correctness?

[kuhar]

I found one meaningful optimization: 20a238e

This seems to be ~10% slower on rccl hotswap translation -- we need to bring the overhead down before this can land.

Does the rccl translation need the EXEC state tracking for correctness?

If we can, we should have a very quick check that answers this and decides whether to skip the analysis or not.

[kuhar]

Original anchor: emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/analysis/exec_state.h:77

Could we make before_ sparse and store only instructions whose incoming state is Full? before() already returns Unknown for missing entries, and liveness only distinguishes the Full case, so an unordered_set<const Instruction *> full_before_ preserves the query contract while avoiding a map node and state value for every instruction.

I prototyped this as local commit 20a238e3de (5 insertions, 4 deletions). On the exact 224,676,512-byte RCCL object, three CPU-32-pinned runs reduced median CPU time from 184.46 s to 171.64 s (-6.95%) and wall time from 184.53 s to 171.72 s (-6.94%). Median peak RSS was effectively flat: 15,977,508 KiB to 15,977,872 KiB (+0.002%). A matched perf stat run showed task-clock -7.51%, cycles -7.93%, instructions -1.88%, branches -1.95%, branch misses -2.96%, and about 71k fewer page faults. All runs produced the same 224,676,512-byte output with SHA-256 7c843d92dddbe69b1944a84eabd3d8d6a64304c6a84f1a03af638a16f7d15d9b; all 76 focused liveness, EXEC, and gfx1250 translation tests passed.