[AMDGPU] Mark v_cmp* convergent

[rampitec]

Summary

This replaces readsExecAsData check with the convergent attribute
to prevent hoisting or sinking which seems to be illegal for vector
compares.

[rampitec]

This is the followup after discussion in the D117909.

[ruiling]

I am not sure this is a good idea. Actually an ordinary V_CMP is still sinkable. It is only the V_CMP lowered from llvm.amdgcn.icmp that cannot be sinked.

[foad]

I am not sure this is a good idea. Actually an ordinary V_CMP is still sinkable. It is only the V_CMP lowered from llvm.amdgcn.icmp that cannot be sinked.

Right, an ordinary V_CMP is still sinkable because of the way the result is used, there is an implicit guarantee that bits in the result corresponding to inactive lanes (at the point of use) will be ignored. So marking *all* V_CMPs convergent is overkill, although it does not seem to have any bad effects on the test suite. On the other hand, duplicating all the V_CMP instructions (so we have separate convergent and non-convergent versions of them) doesn't seem like a good idea, because there are lots of them.

I'm not sure what the best solution is.

[ruiling]

In D118185#3271905, @foad wrote:

I am not sure this is a good idea. Actually an ordinary V_CMP is still sinkable. It is only the V_CMP lowered from llvm.amdgcn.icmp that cannot be sinked.

Right, an ordinary V_CMP is still sinkable because of the way the result is used, there is an implicit guarantee that bits in the result corresponding to inactive lanes (at the point of use) will be ignored. So marking *all* V_CMPs convergent is overkill, although it does not seem to have any bad effects on the test suite. On the other hand, duplicating all the V_CMP instructions (so we have separate convergent and non-convergent versions of them) doesn't seem like a good idea, because there are lots of them.

If duplicating V_CMP for convergent and non-convergent version would make code over-complex, I would go for this simple solution. We can re-visit this issue when we really hit some performance issue here. Let's see if others have different opinion.

[foad]

Maybe another solution is to try to deprecate and remove amdgcn.icmp in favour of amdcgn.ballot, and try to lower ballot to a single machine instruction that is marked "convergent".

[rampitec]

In D118185#3271905, @foad wrote:

Right, an ordinary V_CMP is still sinkable because of the way the result is used, there is an implicit guarantee that bits in the result corresponding to inactive lanes (at the point of use) will be ignored.

I see it that way: technically HW instruction is convergent. If it would not mask the result with exec it would not be convergent. Then you are right we can ignore this fact given the promise of the at least somewhat structured control flow. Then the use of the intrinsic breaks the promise and essentially stepping into the crosslane territory. Although that is not a property of the instruction but rather of its use.

I.e. marking compares as convergent seems to be safe, the other question is if it is desired or not.

[ruiling]

In D118185#3272249, @foad wrote:

Maybe another solution is to try to deprecate and remove amdgcn.icmp in favour of amdcgn.ballot, and try to lower ballot to a single machine instruction that is marked "convergent".

A second look, I agree with @foad using amdgcn.ballot is a good idea. Sounds like we need to fix the existing implementation of amdgcn.ballot as @foad's suggested.

[rampitec]

Abandoning since this is not a way to go.

[critson]

As an aside, I am working on a patch to add a "shouldHoist" to control the behaviour of MachineLICM.
This mostly mirrors the shouldSink that already exists.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

if (isVOP1(MI) || isVOP2(MI) || isVOP3(MI) || isSDWA(MI) || isSALU(MI)) {
     return !MI.hasImplicitDef() &&
            MI.getNumImplicitOperands() == MI.getDesc().getNumImplicitUses() &&
            !MI.mayRaiseFPException();
   }
 
   return false;
 }
 
-static bool readsExecAsData(const MachineInstr &MI) {
-  if (MI.isCompare())
-    return true;
-
-  switch (MI.getOpcode()) {
-  default:
-    break;
-  case AMDGPU::V_READFIRSTLANE_B32:
-    return true;
-  }
-
-  return false;
-}
-
 bool SIInstrInfo::isIgnorableUse(const MachineOperand &MO) const {
   // Any implicit use of exec by VALU is not a real register read.
   return MO.getReg() == AMDGPU::EXEC && MO.isImplicit() &&
-         isVALU(*MO.getParent()) && !readsExecAsData(*MO.getParent());
+         isVALU(*MO.getParent());
 }
 
 bool SIInstrInfo::areLoadsFromSameBasePtr(SDNode *Load0, SDNode *Load1,
                                           int64_t &Offset0,
                                           int64_t &Offset1) const {
   if (!Load0->isMachineOpcode() || !Load1->isMachineOpcode())
     return false;
 

llvm/lib/Target/AMDGPU/VOPCInstructions.td

class VOPC_Pseudo <string opName, VOPC_Profile P, list<dag> pattern=[],
 
   string Mnemonic = opName;
   string AsmOperands = P.Asm32;
 
   let Size = 4;
   let mayLoad = 0;
   let mayStore = 0;
   let hasSideEffects = 0;
+  let isConvergent = 1;
 
   let ReadsModeReg = isFloatType<P.Src0VT>.ret;
 
   let VALU = 1;
   let VOPC = 1;
   let Uses = !if(ReadsModeReg, [MODE, EXEC], [EXEC]);
   let Defs = !if(DefVcc, [VCC], []);
 
multiclass VOPC_Pseudos <string opName,
                          string revOp = opName,
                          bit DefExec = 0> {
 
   def _e32 : VOPC_Pseudo <opName, P>,
              Commutable_REV<revOp#"_e32", !eq(revOp, opName)>,
              VCMPXNoSDstTable<1, opName#"_e32"> {
     let Defs = !if(DefExec, [VCC, EXEC], [VCC]);
     let SchedRW = P.Schedule;
-    let isConvergent = DefExec;
+    let isConvergent = 1;
     let isCompare = 1;
     let isCommutable = 1;
   }
 
   def _e64 : VOP3_Pseudo<opName, P, getVOPCPat64<cond, P>.ret>,
     Commutable_REV<revOp#"_e64", !eq(revOp, opName)>,
     VCMPXNoSDstTable<1, opName#"_e64"> {
     let Defs = !if(DefExec, [EXEC], []);
     let SchedRW = P.Schedule;
     let isCompare = 1;
     let isCommutable = 1;
+    let isConvergent = 1;
   }
 
   foreach _ = BoolToList<P.HasExtSDWA>.ret in
   def _sdwa : VOPC_SDWA_Pseudo <opName, P> {
     let Defs = !if(DefExec, [EXEC], []);
     let SchedRW = P.Schedule;
-    let isConvergent = DefExec;
+    let isConvergent = 1;
     let isCompare = 1;
   }
 }
 
 let SubtargetPredicate = HasSdstCMPX in {
 multiclass VOPCX_Pseudos <string opName,
                           VOPC_Profile P, VOPC_Profile P_NoSDst,
                           SDPatternOperator cond = COND_NULL,
                           string revOp = opName> :
            VOPC_Pseudos <opName, P, cond, revOp, 1> {
 
   def _nosdst_e32 : VOPC_Pseudo <opName#"_nosdst", P_NoSDst, [], 0>,
              Commutable_REV<revOp#"_nosdst_e32", !eq(revOp, opName)>,
              VCMPXNoSDstTable<0, opName#"_e32"> {
     let Defs = [EXEC];
     let SchedRW = P_NoSDst.Schedule;
     let isConvergent = 1;
     let isCompare = 1;
     let isCommutable = 1;
+    let isConvergent = 1;
     let SubtargetPredicate = HasNoSdstCMPX;
   }
 
   def _nosdst_e64 : VOP3_Pseudo<opName#"_nosdst", P_NoSDst>,
     Commutable_REV<revOp#"_nosdst_e64", !eq(revOp, opName)>,
     VCMPXNoSDstTable<0, opName#"_e64"> {
     let Defs = [EXEC];
     let SchedRW = P_NoSDst.Schedule;
     let isCompare = 1;
     let isCommutable = 1;
+    let isConvergent = 1;
     let SubtargetPredicate = HasNoSdstCMPX;
   }
 
   foreach _ = BoolToList<P_NoSDst.HasExtSDWA>.ret in
   def _nosdst_sdwa : VOPC_SDWA_Pseudo <opName#"_nosdst", P_NoSDst> {
     let Defs = [EXEC];
     let SchedRW = P_NoSDst.Schedule;
     let isConvergent = 1;

 // the 1st source operand.
 multiclass VOPC_Class_Pseudos <string opName, VOPC_Profile p, bit DefExec,
                                bit DefVcc = 1> {
   def _e32 : VOPC_Pseudo <opName, p>,
              VCMPXNoSDstTable<1, opName#"_e32"> {
     let Defs = !if(DefExec, !if(DefVcc, [VCC, EXEC], [EXEC]),
                             !if(DefVcc, [VCC], []));
     let SchedRW = p.Schedule;
-    let isConvergent = DefExec;
+    let isConvergent = 1;
   }
 
   def _e64 : VOP3_Pseudo<opName, p, getVOPCClassPat64<p>.ret>,
              VCMPXNoSDstTable<1, opName#"_e64"> {
     let Defs = !if(DefExec, [EXEC], []);
     let SchedRW = p.Schedule;
+    let isConvergent = 1;
   }
 
   foreach _ = BoolToList<p.HasExtSDWA>.ret in
   def _sdwa : VOPC_SDWA_Pseudo <opName, p> {
     let Defs = !if(DefExec, !if(DefVcc, [VCC, EXEC], [EXEC]),
                             !if(DefVcc, [VCC], []));
     let SchedRW = p.Schedule;
-    let isConvergent = DefExec;
+    let isConvergent = 1;
   }
 }
 
 let SubtargetPredicate = HasSdstCMPX in {
 multiclass VOPCX_Class_Pseudos <string opName,
                                 VOPC_Profile P,
                                 VOPC_Profile P_NoSDst> :
            VOPC_Class_Pseudos <opName, P, 1, 1> {
 
   def _nosdst_e32 : VOPC_Pseudo <opName#"_nosdst", P_NoSDst, [], 0>,
                     VCMPXNoSDstTable<0, opName#"_e32"> {
     let Defs = [EXEC];
     let SchedRW = P_NoSDst.Schedule;
     let isConvergent = 1;
     let SubtargetPredicate = HasNoSdstCMPX;
   }
 
   def _nosdst_e64 : VOP3_Pseudo<opName#"_nosdst", P_NoSDst>,
                     VCMPXNoSDstTable<0, opName#"_e64"> {
     let Defs = [EXEC];
     let SchedRW = P_NoSDst.Schedule;
     let SubtargetPredicate = HasNoSdstCMPX;
+    let isConvergent = 1;
   }
 
   foreach _ = BoolToList<P_NoSDst.HasExtSDWA>.ret in
   def _nosdst_sdwa : VOPC_SDWA_Pseudo <opName#"_nosdst", P_NoSDst> {
     let Defs = [EXEC];
     let SchedRW = P_NoSDst.Schedule;
     let isConvergent = 1;
     let SubtargetPredicate = HasNoSdstCMPX;

llvm/test/CodeGen/AMDGPU/undefined-subreg-liverange.ll

 ; CHECK-NEXT:    s_mov_b32 s2, -1
 ; CHECK-NEXT:    buffer_load_dword v1, off, s[0:3], 0 glc
 ; CHECK-NEXT:    s_waitcnt vmcnt(0)
 ; CHECK-NEXT:    buffer_load_dword v0, off, s[0:3], 0 glc
 ; CHECK-NEXT:    s_waitcnt vmcnt(0)
 ; CHECK-NEXT:    v_mov_b32_e32 v2, 0x7fc00000
 ; CHECK-NEXT:    buffer_store_dword v2, off, s[0:3], 0
 ; CHECK-NEXT:    buffer_store_dwordx4 v[0:3], off, s[0:3], 0
-; CHECK-NEXT:    v_cmp_ne_u32_e32 vcc, 0, v1
-; CHECK-NEXT:    s_waitcnt expcnt(0)
-; CHECK-NEXT:    v_cndmask_b32_e64 v1, 0, 1, vcc
-; CHECK-NEXT:    v_cmp_ne_u32_e64 s[0:1], 1, v1
+; CHECK-NEXT:    v_cmp_ne_u32_e64 s[0:1], 0, v1
 ; CHECK-NEXT:  .LBB1_1: ; %bb9
 ; CHECK-NEXT:    ; =>This Inner Loop Header: Depth=1
-; CHECK-NEXT:    s_and_b64 vcc, exec, s[0:1]
+; CHECK-NEXT:    s_andn2_b64 vcc, exec, s[0:1]
 ; CHECK-NEXT:    s_cbranch_vccnz .LBB1_1
 ; CHECK-NEXT:  ; %bb.2: ; %bb11
 ; CHECK-NEXT:    s_mov_b32 s3, 0xf000
 ; CHECK-NEXT:    s_mov_b32 s2, -1
 ; CHECK-NEXT:    buffer_store_dwordx4 v[0:3], off, s[0:3], 0
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0)
 ; CHECK-NEXT:    ; return to shader part epilog
 bb: