[AMDGPU] Add missing use check in SIOptimizeExecMasking pass.

[tsymalla]

Summary

Whenever a v_cmp, s_and_saveexec instruction sequence shall be
transformed to an equivalent s_mov, v_cmpx sequence, it needs
to be detected if the v_cmp target register is used between
the two instructions as the v_cmp result gets omitted by
using the v_cmpx instruction, resulting in invalid code.

[tsymalla]

Fix start iterator usage.

[tsymalla]

Improve boolean flag usage for register liveness checks.

[tsymalla]

Remove .s file from commit.

[foad]

Looks OK as it fixes a bug, but I think the liveness checking could be cleaned up as noted inline.

llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp

414–422

I'm not sure that the isRegisterInUseBetween and isRegisterInUseAfter helper functions are actually making this code any simpler, or easier to understand.

The conditions you really want to check are:

1. is VCmpDest->getReg() live immediately after SaveExec? You can do this with LivePhysRegs, and this check would replace both the isLiveOut check (line 414) and the isRegisterInUseAfter check.
2. is VCmpDest->getReg() used by an instruction between *VCmp and SaveExec? You don't need any kind of liveness for this check, you can just examine the operands of every instruction in that range.

llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp

for (++A; CurrentIteration < MaxInstructions && A != E; ++A) {
     }
 
     ++CurrentIteration;
   }
 
   return nullptr;
 }
 
+
 // Determine if a register Reg is not re-defined and still in use
-// in the range (Stop..BB.end].
+// in the range (Stop..Start].
 // It does so by backwards calculating liveness from the end of the BB until
 // either Stop or the beginning of the BB is reached.
 // After liveness is calculated, we can determine if Reg is still in use and not
 // defined inbetween the instructions.
-static bool isRegisterInUseAfter(MachineInstr &Stop, MCRegister Reg,
-                                 const SIRegisterInfo *TRI,
-                                 MachineRegisterInfo &MRI) {
+static bool isRegisterInUseBetween(MachineInstr &Stop, MachineInstr &Start,
+                                   MCRegister Reg, const SIRegisterInfo *TRI,
+                                   MachineRegisterInfo &MRI,
+                                   bool useLiveOuts = false,
+                                   bool ignoreStart = false) {
   LivePhysRegs LR(*TRI);
+  if (useLiveOuts)
     LR.addLiveOuts(*Stop.getParent());
 
-  for (auto A = Stop.getParent()->rbegin();
-       A != Stop.getParent()->rend() && A != Stop; ++A) {
+  MachineBasicBlock::reverse_iterator A(Start);
+  MachineBasicBlock::reverse_iterator E(Stop);
+
+  if (ignoreStart)
+    ++A;
+
+  for (; A != Stop.getParent()->rend() && A != Stop; ++A) {
     LR.stepBackward(*A);
   }
 
   return !LR.available(MRI, Reg);
 }
 
+// Determine if a register Reg is not re-defined and still in use
+// in the range (Stop..BB.end].
+static bool isRegisterInUseAfter(MachineInstr &Stop, MCRegister Reg,
+                                 const SIRegisterInfo *TRI,
+                                 MachineRegisterInfo &MRI) {
+  return isRegisterInUseBetween(Stop, *Stop.getParent()->rbegin(), Reg, TRI,
+                                MRI, true);
+}
+
 // Tries to find a possibility to optimize a v_cmp ..., s_and_saveexec sequence
 // by looking at an instance of a s_and_saveexec instruction. Returns a pointer
 // to the v_cmp instruction if it is safe to replace the sequence (see the
 // conditions in the function body). This is after register allocation, so some
 // checks on operand dependencies need to be considered.
 static MachineInstr *findPossibleVCMPVCMPXOptimization(
     MachineInstr &SaveExec, MCRegister Exec, const SIRegisterInfo *TRI,
     const SIInstrInfo *TII, MachineRegisterInfo &MRI) {
static MachineInstr *findPossibleVCMPVCMPXOptimization(
   if (Src1->isReg() && TRI->isSGPRReg(MRI, Src1->getReg()) &&
       SaveExec.modifiesRegister(Src1->getReg(), TRI))
     return nullptr;
 
   // Don't do the transformation if the destination operand is included in
   // it's MBB Live-outs, meaning it's used in any of it's successors, leading
   // to incorrect code if the v_cmp and therefore the def of
   // the dest operand is removed.
   if (isLiveOut(*VCmp->getParent(), VCmpDest->getReg()))
     return nullptr;
 
-  // If the v_cmp target is in use after the s_and_saveexec, skip the
-  // optimization.
-  if (isRegisterInUseAfter(SaveExec, VCmpDest->getReg(), TRI, MRI))
+  // If the v_cmp target is in use between v_cmp and s_and_saveexec or after the
+  // s_and_saveexec, skip the optimization.
+  if (isRegisterInUseBetween(*VCmp, SaveExec, VCmpDest->getReg(), TRI, MRI,
+                             false, true) ||
+      isRegisterInUseAfter(SaveExec, VCmpDest->getReg(), TRI, MRI))
     return nullptr;
 
   // Try to determine if there is a write to any of the VCmp
   // operands between the saveexec and the vcmp.
   // If yes, additional VGPR spilling might need to be inserted. In this case,
   // it's not worth replacing the instruction sequence.
   SmallVector<MCRegister, 2> NonDefRegs;
   if (Src0->isReg())
     NonDefRegs.push_back(Src0->getReg());

llvm/test/CodeGen/AMDGPU/vcmp-saveexec-to-vcmpx.mir

bb.0:
     liveins: $vgpr0, $sgpr2
     renamable $sgpr0_sgpr1 = V_CMP_GT_U32_e64 $sgpr2, killed $vgpr0, implicit $exec
     $sgpr2_sgpr3 = COPY $exec, implicit-def $exec
     $sgpr2_sgpr3 = S_AND_B64 killed renamable $sgpr2_sgpr3, killed renamable $sgpr0_sgpr1, implicit-def dead $scc
     $exec = S_MOV_B64_term killed renamable $sgpr2_sgpr3
 ...
 
 ---
+
+# Ensure the transformation does not get applied when the v_cmp target is used before the s_and_saveexec instruction.
+
+# GCN-LABEL: name: vcmp_saveexec_to_mov_vcmpx_exec_intermediate_use
+# GCN: V_CMP_LT_F32_e64
+# GCN: V_WRITELANE_B32
+# GCN: S_AND_SAVEEXEC_B64
+name: vcmp_saveexec_to_mov_vcmpx_exec_intermediate_use
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $vgpr0, $vgpr1, $sgpr2
+    renamable $sgpr0_sgpr1 = V_CMP_LT_F32_e64 0, 953267991, 2, $vgpr1, 0, implicit $mode, implicit $exec
+    $vgpr0 = V_WRITELANE_B32 0, $sgpr0, $vgpr0
+    $sgpr2_sgpr3 = COPY $exec, implicit-def $exec
+    $sgpr2_sgpr3 = S_AND_B64 killed renamable $sgpr2_sgpr3, killed renamable $sgpr0_sgpr1, implicit-def dead $scc
+    $exec = S_MOV_B64_term killed renamable $sgpr2_sgpr3
+...
+
+---
+
 # Check if the modifiers are preserved when generating the V_CMPX instruction.
 
 # GCN-LABEL: name: vcmp_saveexec_to_mov_vcmpx_check_abs
 # GFX1010: V_CMP_LT_F32_e64
 # GFX1010: S_AND_SAVEEXEC_B64
 # GFX1030: S_MOV_B64
 # GFX1030-NEXT: V_CMPX_LT_F32_nosdst_e64 0, 953267991, 2
 name: vcmp_saveexec_to_mov_vcmpx_check_abs
 tracksRegLiveness: true
 body: |
   bb.0:
     liveins: $vgpr0
     renamable $sgpr0_sgpr1 = V_CMP_LT_F32_e64 0, 953267991, 2, $vgpr0, 0, implicit $mode, implicit $exec
     $sgpr2_sgpr3 = COPY $exec, implicit-def $exec
     $sgpr2_sgpr3 = S_AND_B64 killed renamable $sgpr2_sgpr3, killed renamable $sgpr0_sgpr1, implicit-def dead $scc
     $exec = S_MOV_B64_term killed renamable $sgpr2_sgpr3