[AMDGPU] SIWholeQuadMode: avoid execz effects in exact regions

[perlfu]

Exact mode regions within WQM may have EXEC=0 in divergent control flow. This occurs if a branch is only taken by helper lanes and an instruction requiring WQM disabling is encountered.

The current code extends the exact region as far as possible; however, this can result in it including instructions with unwanted side effects at EXEC=0.
In particular readfirstlane combined with scalar loads can produce invalid memory accesses in this circumstance.

Workaround this by shrinking exact regions to only the instructions requiring WQM disabling when unwanted side effects are present. Eventually we should branch over these regions when EXEC=0, but this requires visibility of CFG/divergence information not currently available.

[shanyizhang]

This change can fix an issue of a Vulkan game title.

[piotrAMD]

Can you pre-commit the test?

The patch looks good to me. The bug makes me slightly worried that there might be other cases with exec = 0 that we are not detecting in the pass.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Carl Ritson (perlfu)

Changes

Exact mode regions within WQM may have EXEC=0 in divergent control flow. This occurs if a branch is only taken by helper lanes and an instruction requiring WQM disabling is encountered.

The current code extends the exact region as far as possible; however, this can result in it including instructions with unwanted side effects at EXEC=0.
In particular readfirstlane combined with scalar loads can produce invalid memory accesses in this circumstance.

Workaround this by shrinking exact regions to only the instructions requiring WQM disabling when unwanted side effects are present. Eventually we should branch over these regions when EXEC=0, but this requires visibility of CFG/divergence information not currently available.

---

Full diff: https://github.com/llvm/llvm-project/pull/101157.diff

2 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp (+21-5)
• (modified) llvm/test/CodeGen/AMDGPU/wqm.ll (+8-8)

diff --git a/llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp b/llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp
index df7906ebd8a7e..9a51cbbb9f6b8 100644
--- a/llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp
+++ b/llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp
@@ -1364,8 +1364,26 @@ void SIWholeQuadMode::processBlock(MachineBasicBlock &MBB, bool IsEntry) {
         llvm_unreachable("Unknown state");
         break;
       }
+      char StartState = State & StateStrict ? NonStrictState : State;
+      bool WQMToExact =
+          StartState == StateWQM && (Needs & StateExact) && !(Needs & StateWQM);
+      bool ExactToWQM = StartState == StateExact && (Needs & StateWQM) &&
+                        !(Needs & StateExact);
+      bool PreferLast = Needs == StateWQM;
+      // Exact regions in divergent control flow may run at EXEC=0, so try to
+      // exclude instructions with unexpected effects from them.
+      // FIXME: ideally we would branch over these when EXEC=0,
+      // but this requires updating implicit values, live intervals and CFG.
+      if ((WQMToExact && (OutNeeds & StateWQM)) || ExactToWQM) {
+        for (MachineBasicBlock::iterator I = First; I != II; ++I) {
+          if (TII->hasUnwantedEffectsWhenEXECEmpty(*I)) {
+            PreferLast = WQMToExact;
+            break;
+          }
+        }
+      }
       MachineBasicBlock::iterator Before =
-          prepareInsertion(MBB, First, II, Needs == StateWQM, SaveSCC);
+          prepareInsertion(MBB, First, II, PreferLast, SaveSCC);
 
       if (State & StateStrict) {
         assert(State == StateStrictWWM || State == StateStrictWQM);
@@ -1385,9 +1403,8 @@ void SIWholeQuadMode::processBlock(MachineBasicBlock &MBB, bool IsEntry) {
 
         toStrictMode(MBB, Before, SavedNonStrictReg, Needs);
         State = Needs;
-
       } else {
-        if (State == StateWQM && (Needs & StateExact) && !(Needs & StateWQM)) {
+        if (WQMToExact) {
           if (!WQMFromExec && (OutNeeds & StateWQM)) {
             assert(!SavedWQMReg);
             SavedWQMReg = MRI->createVirtualRegister(BoolRC);
@@ -1395,8 +1412,7 @@ void SIWholeQuadMode::processBlock(MachineBasicBlock &MBB, bool IsEntry) {
 
           toExact(MBB, Before, SavedWQMReg);
           State = StateExact;
-        } else if (State == StateExact && (Needs & StateWQM) &&
-                   !(Needs & StateExact)) {
+        } else if (ExactToWQM) {
           assert(WQMFromExec == (SavedWQMReg == 0));
 
           toWQM(MBB, Before, SavedWQMReg);
diff --git a/llvm/test/CodeGen/AMDGPU/wqm.ll b/llvm/test/CodeGen/AMDGPU/wqm.ll
index ee71502cdd94e..6b4c2da772cdc 100644
--- a/llvm/test/CodeGen/AMDGPU/wqm.ll
+++ b/llvm/test/CodeGen/AMDGPU/wqm.ll
@@ -3551,13 +3551,13 @@ define amdgpu_ps float @short_exact_regions(<8 x i32> inreg %rsrc, <4 x i32> inr
 ; GFX9-W64-NEXT:    s_and_saveexec_b64 s[14:15], vcc
 ; GFX9-W64-NEXT:    s_cbranch_execz .LBB59_2
 ; GFX9-W64-NEXT:  ; %bb.1: ; %if
-; GFX9-W64-NEXT:    s_and_saveexec_b64 s[16:17], s[12:13]
 ; GFX9-W64-NEXT:    global_load_dword v0, v[1:2], off
 ; GFX9-W64-NEXT:    s_waitcnt vmcnt(0)
-; GFX9-W64-NEXT:    v_readfirstlane_b32 s18, v0
-; GFX9-W64-NEXT:    s_buffer_load_dword s18, s[8:11], s18 offset:0x0
+; GFX9-W64-NEXT:    v_readfirstlane_b32 s16, v0
+; GFX9-W64-NEXT:    s_buffer_load_dword s16, s[8:11], s16 offset:0x0
 ; GFX9-W64-NEXT:    s_waitcnt lgkmcnt(0)
-; GFX9-W64-NEXT:    v_mov_b32_e32 v0, s18
+; GFX9-W64-NEXT:    v_mov_b32_e32 v0, s16
+; GFX9-W64-NEXT:    s_and_saveexec_b64 s[16:17], s[12:13]
 ; GFX9-W64-NEXT:    buffer_store_dwordx4 v[3:6], v0, s[0:3], 0 idxen
 ; GFX9-W64-NEXT:    s_mov_b64 exec, s[16:17]
 ; GFX9-W64-NEXT:  .LBB59_2: ; %endif
@@ -3581,13 +3581,13 @@ define amdgpu_ps float @short_exact_regions(<8 x i32> inreg %rsrc, <4 x i32> inr
 ; GFX10-W32-NEXT:    v_cmpx_gt_u32_e32 16, v0
 ; GFX10-W32-NEXT:    s_cbranch_execz .LBB59_2
 ; GFX10-W32-NEXT:  ; %bb.1: ; %if
-; GFX10-W32-NEXT:    s_and_saveexec_b32 s14, s12
 ; GFX10-W32-NEXT:    global_load_dword v0, v[1:2], off
 ; GFX10-W32-NEXT:    s_waitcnt vmcnt(0)
-; GFX10-W32-NEXT:    v_readfirstlane_b32 s15, v0
-; GFX10-W32-NEXT:    s_buffer_load_dword s15, s[8:11], s15 offset:0x0
+; GFX10-W32-NEXT:    v_readfirstlane_b32 s14, v0
+; GFX10-W32-NEXT:    s_buffer_load_dword s14, s[8:11], s14 offset:0x0
 ; GFX10-W32-NEXT:    s_waitcnt lgkmcnt(0)
-; GFX10-W32-NEXT:    v_mov_b32_e32 v0, s15
+; GFX10-W32-NEXT:    v_mov_b32_e32 v0, s14
+; GFX10-W32-NEXT:    s_and_saveexec_b32 s14, s12
 ; GFX10-W32-NEXT:    buffer_store_dwordx4 v[3:6], v0, s[0:3], 0 idxen
 ; GFX10-W32-NEXT:    s_mov_b32 exec_lo, s14
 ; GFX10-W32-NEXT:  .LBB59_2: ; %endif

[perlfu]

• Rebased on to pre-commit tests
• Tighten the condition for shortening exact regions - makes change more targeted and thus has essentially no impact on any existing shaders unimpacted by this issue

In the main we might possibly have some more missed EXECZ regions, but they can only be from WQM to exact transitions.
Strict WWM enables extra lanes for short sections, so will never be EXECZ.
Strict WQM can only be EXECZ if it was entered into from an existing EXECZ region, so in principle this might be possible.
This would have to be full shader WQM -> exact -> strict WQM -- which is low probability.

Again I think this is probably best addressed after control flow lowering is reworked (i.e. wave transform).

[jayfoad]

@perlfu @shanyizhang is this worth backporting to 19.x?

[shanyizhang]

Sorry for late response. I didn't know exactly what 19.x is. My request here is to have this fix in XGL as soon as possible.