Wave Transform to generate SSA Exec mask manipulation instrs #789

lalaniket8 · 2025-12-08T11:26:04Z

Since we are moving Wave Transform to the middle of Register Allocation after PHI-elimination, the Exec Mask Manipulation instructions added to the code by Wave Transform should not be in SSA.
This PR contains code changes to support this.
We remove SSAUpdater originally used and used a single Accumulator Register to capture contributions from Thread-level CFG predecessors of a basic block. This Accumulator is used to set the appropriate EXEC mask. The Reset Flag Semantics of GCNLaneMaskUpdater is retained and used to reset the Accumulator at correct points in the code.

z1-cciauto · 2025-12-08T11:26:34Z

Failed to trigger build:

github-actions · 2025-12-08T11:26:36Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

cdevadas

Have you done the clang-format? Felt like at some places the format wasn't good.

llvm/lib/CodeGen/MachineRegisterInfo.cpp

cdevadas · 2025-12-09T10:26:17Z

llvm/include/llvm/CodeGen/MachineRegisterInfo.h

+  /// getDomVRegDefInBasicBlock - Return the last machine instr that defines
+  /// the specified virtual register in the basic block, searching backwards
+  /// from instruction I (exclusive). Returns MBB.end() if no definition is found.
+  LLVM_ABI MachineBasicBlock::iterator getDomVRegDefInBasicBlock(Register Reg, MachineBasicBlock &MBB, MachineBasicBlock::iterator I) const;


I'm not sure this should go to the MRI. This function is for a very specific usecase and used currently only for AMDGPU. SIRegisterInfo.cpp seems ok to me.

llvm/lib/Target/AMDGPU/GCNLaneMaskUtils.cpp

z1-cciauto · 2025-12-09T11:00:41Z

Failed to trigger build:

lalaniket8 · 2025-12-09T11:06:44Z

Have you done the clang-format? Felt like at some places the format wasn't good.

Addressed in latest commit

cmc-rep · 2025-12-09T17:34:01Z

I have started reviewing the code change . In the meantime,
We have provided multiple tests under llvm/test/CodeGen/AMDGPU, could we update those tests?

For those ll files, we want to add the run-line to stop after wave-transform, and check generated MIR.
For those MIR files, manually update them to Non-SSA form, and run wave-transform pass.

Actually, for those ll files, I would suggest that we first have a separate PR to add those run-line and check-result showing what the MIR look like right before wave-transform. Hopefully, those tests are easy to add, they can get merged before this PR.
This PR then will update those tests with the result after wave-transform. This way, we can compare the MIR before and after wave-transform during code review.

cmc-rep · 2025-12-09T22:02:18Z

llvm/lib/CodeGen/MachineRegisterInfo.cpp

-  Register Reg, MachineBasicBlock &MBB, MachineBasicBlock::iterator I) const {
-  if(I == MBB.begin()) return MBB.end();
+    Register Reg, MachineBasicBlock &MBB, MachineBasicBlock::iterator I) const {
+  if (I == MBB.begin())


This code can be simplified into a while (I != MBB.begin()) { .... } loop, right? also return a pointer of MachineInstr seems more straightforward?

Yep. Turn it into a for/while loop and add this condition check as the loop terminator. I second that idea of returning a MachineInstr*. All the machinery of checking I != MBB.end() at the call-sites (after this function returns) can be simplified with a !MI.

cmc-rep · 2025-12-09T22:09:45Z

Also please make sure all the code comments are up to date with the code changes. For example, any comment mentioning PHI node is likely out of date.

Also I feel thtat we should clean up LaneMaskUtil code that does not really get used. For example, for our application, we always assume accumulating == true. If we are not going to maintain the code that assumes accumulating == false, we may want to delete them. I personally would prefer getting the code as simpler as possible

llvm/lib/Target/AMDGPU/GCNLaneMaskUtils.cpp

cdevadas · 2025-12-10T04:29:51Z

I have started reviewing the code change . In the meantime, We have provided multiple tests under llvm/test/CodeGen/AMDGPU, could we update those tests?

I thought about that initially. Once this patch gets merged, the next patch will be to enable wave-transform by default, and that would cover all lit tests in the new pipeline. At the moment, most lit tests would break if wave transform is force-enabled as the original implementation depends on the SSAUpdater and introduces PHI nodes. However, it makes sense to add some selected tests to verify the new wave-transform changes.

For those ll files, we want to add the run-line to stop after wave-transform, and check generated MIR.

Better to select some control-flow tests involving loops and if-else and stop-after wave-transform pass. @lalaniket8 can you identify some tests and pre-commit the new changes?

For those MIR files, manually update them to Non-SSA form, and run wave-transform pass.

Actually, for those ll files, I would suggest that we first have a separate PR to add those run-line and check-result showing what the MIR look like right before wave-transform. Hopefully, those tests are easy to add, they can get merged before this PR. This PR then will update those tests with the result after wave-transform. This way, we can compare the MIR before and after wave-transform during code review.

lalaniket8 · 2025-12-10T04:36:25Z

Also please make sure all the code comments are up to date with the code changes. For example, any comment mentioning PHI node is likely out of date.

Should we also remove the SSAReconstructor class in AMDGPUWaveTransform.cpp since that is not needed anymore?

Also I feel thtat we should clean up LaneMaskUtil code that does not really get used. For example, for our application, we always assume accumulating == true. If we are not going to maintain the code that assumes accumulating == false, we may want to delete them. I personally would prefer getting the code as simpler as possible

Yes, I think it a good idea to remove the Default mode and keep only Accumulating mode, it will simplify the code a lot.
Should I have another PR for cleaning up this part, or a commit in this PR?

cdevadas · 2025-12-10T06:11:35Z

Also please make sure all the code comments are up to date with the code changes. For example, any comment mentioning PHI node is likely out of date.

Should we also remove the SSAReconstructor class in AMDGPUWaveTransform.cpp since that is not needed anymore?

Also I feel thtat we should clean up LaneMaskUtil code that does not really get used. For example, for our application, we always assume accumulating == true. If we are not going to maintain the code that assumes accumulating == false, we may want to delete them. I personally would prefer getting the code as simpler as possible

Yes, I think it a good idea to remove the Default mode and keep only Accumulating mode, it will simplify the code a lot. Should I have another PR for cleaning up this part, or a commit in this PR?

You can add the clean up in this PR itself.

cdevadas · 2025-12-10T06:23:29Z

Also please make sure all the code comments are up to date with the code changes. For example, any comment mentioning PHI node is likely out of date.

Should we also remove the SSAReconstructor class in AMDGPUWaveTransform.cpp since that is not needed anymore?

How about the second part of the SSAReconstructor that deals with the dominance relation between defs and their respective uses?" Keep it for now. Anyway, we disabled the SSAReconstructor.run() invocation for now. Let's see if there is any fixup needed later when we turn on the wave-transform pipeline by default.

z1-cciauto · 2025-12-10T09:27:36Z

Failed to trigger build:

cmc-rep · 2025-12-10T16:02:00Z

Cleanup looks good to me.

In terms of testing, I was suggesting that we first add run-line for those LL tests to STOP-BEFORE wave-transform in a separate PR, I expect that should works (not crashing). If we can add more tests for more control-flow situations, that would be even better.

In this PR, we should try to turn those STOP-BEFORE into STOP-after. We got multiple people here to examine those test results to ensure correctness, which should be a good and healthy exercise.

vg0204 · 2025-12-11T08:26:26Z

llvm/lib/CodeGen/MachineRegisterInfo.cpp

+  // Iterate backwards from I (exclusive) to the beginning of the basic block
+  do {
+    --I;
+    if (I->modifiesRegister(Reg, getTargetRegisterInfo()))


Suggested change

if (I->modifiesRegister(Reg, getTargetRegisterInfo()))

if (I->definesRegister(Reg, getTargetRegisterInfo()))

If you looking for those instructions only that fully define Reg use the above suggestion because the existing one also accounts for the case of partial definition!

vg0204 · 2025-12-11T08:35:38Z

llvm/include/llvm/CodeGen/MachineRegisterInfo.h

+  /// getDomVRegDefInBasicBlock - Return the last machine instr that defines
+  /// the specified virtual register in the basic block, searching backwards
+  /// from instruction I (exclusive). Returns MBB.end() if no definition is
+  /// found.


The comment seems bit misleading, as the last instruction while searching backward implies the very first definition in the MBB. I think you can separate the two lines : the last instruction encountered in the MBB & you search in the backward manner from I.

vg0204 · 2025-12-11T08:53:21Z

llvm/lib/Target/AMDGPU/GCNLaneMaskUtils.cpp

+        I--;
+      BuildMI(*B, I, {}, TII->get(LMU.getLaneMaskConsts().MovOpc), ACC)
+          .addImm(0);
+    }


Can't you insert all resets one after another once you find the right place rather than searching for right insertion place for every accumulator to reset? Seems bit expensive!

Wave Transform should generate non SSA Exec mask manipulation instrs

10c69e3

lalaniket8 changed the title ~~Wave Transform should generate non SSA Exec mask manipulation instrs~~ Wave Transform to generate SSA Exec mask manipulation instrs Dec 9, 2025

lalaniket8 marked this pull request as ready for review December 9, 2025 05:20

lalaniket8 requested review from cdevadas and nhaehnle December 9, 2025 05:21

cdevadas reviewed Dec 9, 2025

View reviewed changes

lalaniket8 requested review from cmc-rep, jmmartinez and vg0204 December 9, 2025 10:47

sanitized with git clang format and minor fixes

54b5f3b

cmc-rep reviewed Dec 9, 2025

View reviewed changes

cmc-rep reviewed Dec 10, 2025

View reviewed changes

llvm/lib/Target/AMDGPU/GCNLaneMaskUtils.cpp Show resolved Hide resolved

Removed default mode and cleanup

7a31f7f

vg0204 reviewed Dec 11, 2025

View reviewed changes

	if (I->modifiesRegister(Reg, getTargetRegisterInfo()))
	if (I->definesRegister(Reg, getTargetRegisterInfo()))

Wave Transform to generate SSA Exec mask manipulation instrs #789

Are you sure you want to change the base?

Wave Transform to generate SSA Exec mask manipulation instrs #789

Uh oh!

Conversation

lalaniket8 commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

z1-cciauto commented Dec 8, 2025

Uh oh!

github-actions bot commented Dec 8, 2025

Uh oh!

cdevadas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cdevadas Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

z1-cciauto commented Dec 9, 2025

Uh oh!

lalaniket8 commented Dec 9, 2025

Uh oh!

cmc-rep commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmc-rep Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

cdevadas Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

cmc-rep commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cdevadas commented Dec 10, 2025

Uh oh!

lalaniket8 commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdevadas commented Dec 10, 2025

Uh oh!

cdevadas commented Dec 10, 2025

Uh oh!

z1-cciauto commented Dec 10, 2025

Uh oh!

cmc-rep commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vg0204 Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

vg0204 Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

vg0204 Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

vg0204 Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lalaniket8 commented Dec 8, 2025 •

edited

Loading

cmc-rep commented Dec 9, 2025 •

edited

Loading

cmc-rep commented Dec 9, 2025 •

edited

Loading

lalaniket8 commented Dec 10, 2025 •

edited

Loading

cmc-rep commented Dec 10, 2025 •

edited

Loading

vg0204 Dec 11, 2025 •

edited

Loading