- Address reviewer comments.

perlfu · perlfu · commit 44afd5f0f754 · 2024-09-05T10:52:50.000+09:00
- Add findImplicitExecSrc helper. - Use helper to ignore V_SET_INACTIVE instructions during WQM/WWM processing. This allows other passes to emit V_SET_INACTIVE for already known WWM sections. This supports #105822. - Add test for above.
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -2098,8 +2098,20 @@ unsigned SIInstrInfo::getNumWaitStates(const MachineInstr &MI) {
   }
 }
 
+Register SIInstrInfo::findImplicitExecSrc(const MachineInstr &MI) {
+  for (auto &Op : MI.implicit_operands()) {
+    if (Op.isDef())
+      continue;
+    Register OpReg = Op.getReg();
+    if (OpReg == AMDGPU::EXEC || OpReg == AMDGPU::EXEC_LO ||
+        OpReg == AMDGPU::SCC)
+      continue;
+    return OpReg;
+  }
+  return Register();
+}
+
 bool SIInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
-  const SIRegisterInfo *TRI = ST.getRegisterInfo();
   MachineBasicBlock &MBB = *MI.getParent();
   DebugLoc DL = MBB.findDebugLoc(MI);
   switch (MI.getOpcode()) {
@@ -2286,21 +2298,12 @@ bool SIInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
     MachineOperand &InactiveSrc = MI.getOperand(2);
 
     // Find implicit register defining lanes active outside WWM.
+    Register ExecSrcReg = findImplicitExecSrc(MI);
+    assert(ExecSrcReg && "V_SET_INACTIVE must be in known WWM region");
     // Note: default here is set to ExecReg so that functional MIR is still
     // generated if implicit def is not found and assertions are disabled.
-    Register ExecSrcReg = ExecReg;
-    for (auto &Op : MI.implicit_operands()) {
-      if (Op.isDef() || !Op.isReg())
-        continue;
-      Register OpReg = Op.getReg();
-      if (OpReg == AMDGPU::EXEC || OpReg == AMDGPU::EXEC_LO ||
-          OpReg == AMDGPU::SCC)
-        continue;
-      ExecSrcReg = OpReg;
-      break;
-    }
-    assert(ExecSrcReg != ExecReg &&
-           "V_SET_INACTIVE must be in known WWM region");
+    if (!ExecSrcReg)
+      ExecSrcReg = ExecReg;
 
     // Ideally in WWM this operation is lowered to V_CNDMASK; however,
     // constant bus constraints and the presence of literal constants
@@ -2329,20 +2332,26 @@ bool SIInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
         (usesConstantBus(MRI, ActiveSrc, Desc.operands()[Src1Idx]) ? 1 : 0) +
         (usesConstantBus(MRI, InactiveSrc, Desc.operands()[Src0Idx]) ? 1 : 0);
     int LiteralConstants =
-        (ActiveSrc.isImm() && !isInlineConstant(ActiveImm) ? 1 : 0) +
-        (InactiveSrc.isImm() && !isInlineConstant(InactiveImm) ? 1 : 0);
+        ((ActiveSrc.isReg() ||
+          (ActiveSrc.isImm() && isInlineConstant(ActiveImm)))
+             ? 0
+             : 1) +
+        ((InactiveSrc.isReg() ||
+          (InactiveSrc.isImm() && isInlineConstant(InactiveImm)))
+             ? 0
+             : 1);
 
     bool UseVCndMask =
         ConstantBusUses <= ConstantBusLimit && LiteralConstants <= LiteralLimit;
     if (VMov64 && UseVCndMask) {
       // Decomposition must not introduce new literals.
       UseVCndMask &=
           ActiveSrc.isReg() ||
-          (isInlineConstant(ActiveImmLo) && isInlineConstant(ActiveImmLo)) ||
+          (isInlineConstant(ActiveImmLo) && isInlineConstant(ActiveImmHi)) ||
           (!isInlineConstant(ActiveImm));
       UseVCndMask &= InactiveSrc.isReg() ||
                      (isInlineConstant(InactiveImmLo) &&
-                      isInlineConstant(InactiveImmLo)) ||
+                      isInlineConstant(InactiveImmHi)) ||
                      (!isInlineConstant(InactiveImm));
     }
 
@@ -2352,34 +2361,34 @@ bool SIInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
           ActiveSrc.isReg()
               ? MachineOperand::CreateReg(
                     RI.getSubReg(ActiveSrc.getReg(), AMDGPU::sub0), false,
-                    /*isImp=*/false, /*isKill*/ false)
+                    /*isImp=*/false, /*isKill=*/false)
               : MachineOperand::CreateImm(ActiveImmLo.getSExtValue());
       MachineOperand ActiveHi =
           ActiveSrc.isReg()
               ? MachineOperand::CreateReg(
                     RI.getSubReg(ActiveSrc.getReg(), AMDGPU::sub1), false,
-                    /*isImp=*/false, /*isKill*/ ActiveSrc.isKill())
+                    /*isImp=*/false, /*isKill=*/ActiveSrc.isKill())
               : MachineOperand::CreateImm(ActiveImmHi.getSExtValue());
       MachineOperand InactiveLo =
           InactiveSrc.isReg()
               ? MachineOperand::CreateReg(
                     RI.getSubReg(InactiveSrc.getReg(), AMDGPU::sub0), false,
-                    /*isImp=*/false, /*isKill*/ false)
+                    /*isImp=*/false, /*isKill=*/false)
               : MachineOperand::CreateImm(InactiveImmLo.getSExtValue());
       MachineOperand InactiveHi =
           InactiveSrc.isReg()
               ? MachineOperand::CreateReg(
                     RI.getSubReg(InactiveSrc.getReg(), AMDGPU::sub1), false,
-                    /*isImp=*/false, /*isKill*/ InactiveSrc.isKill())
+                    /*isImp=*/false, /*isKill=*/InactiveSrc.isKill())
               : MachineOperand::CreateImm(InactiveImmHi.getSExtValue());
-      BuildMI(MBB, MI, DL, get(Opcode), RI.getSubReg(DstReg, AMDGPU::sub0))
+      BuildMI(MBB, MI, DL, Desc, RI.getSubReg(DstReg, AMDGPU::sub0))
           .addImm(0)
           .add(InactiveLo)
           .addImm(0)
           .add(ActiveLo)
           .addReg(ExecSrcReg)
           .addReg(DstReg, RegState::ImplicitDefine);
-      BuildMI(MBB, MI, DL, get(Opcode), RI.getSubReg(DstReg, AMDGPU::sub1))
+      BuildMI(MBB, MI, DL, Desc, RI.getSubReg(DstReg, AMDGPU::sub1))
           .addImm(0)
           .add(InactiveHi)
           .addImm(0)
@@ -2388,7 +2397,7 @@ bool SIInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
           .addReg(DstReg, RegState::ImplicitDefine);
     } else if (UseVCndMask) {
       // Single V_CNDMASK_B32
-      BuildMI(MBB, MI, DL, get(Opcode), DstReg)
+      BuildMI(MBB, MI, DL, Desc, DstReg)
           .addImm(0)
           .add(InactiveSrc)
           .addImm(0)
@@ -2406,9 +2415,9 @@ bool SIInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
         // Set exec mask to inactive lanes,
         // but only if active lanes would be overwritten.
         if (DstIsActive) {
-          MachineInstr *ExecMI =
-              BuildMI(MBB, MI, DL, get(NotOpc), ExecReg).addReg(ExecSrcReg);
-          ExecMI->addRegisterDead(AMDGPU::SCC, TRI); // SCC is overwritten
+          BuildMI(MBB, MI, DL, get(NotOpc), ExecReg)
+              .addReg(ExecSrcReg)
+              .setOperandDead(3); // Dead scc
         }
         // Copy inactive lanes
         MachineInstr *VMov =
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.h b/llvm/lib/Target/AMDGPU/SIInstrInfo.h
@@ -1437,6 +1437,8 @@ class SIInstrInfo final : public AMDGPUGenInstrInfo {
   // This is used if an operand is a 32 bit register but needs to be aligned
   // regardless.
   void enforceOperandRCAlignment(MachineInstr &MI, unsigned OpName) const;
+
+  static Register findImplicitExecSrc(const MachineInstr &MI);
 };
 
 /// \brief Returns true if a reg:subreg pair P has a TRC class
diff --git a/llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp b/llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp
@@ -559,18 +559,24 @@ char SIWholeQuadMode::scanInstructions(MachineFunction &MF,
         GlobalFlags |= StateStrictWQM;
       } else if (Opcode == AMDGPU::V_SET_INACTIVE_B32 ||
                  Opcode == AMDGPU::V_SET_INACTIVE_B64) {
-        // Disable strict states; StrictWQM will be added as required later.
-        III.Disabled = StateStrict;
-        MachineOperand &Inactive = MI.getOperand(2);
-        if (Inactive.isReg()) {
-          if (Inactive.isUndef()) {
-            LowerToCopyInstrs.insert(&MI);
-          } else {
-            markOperand(MI, Inactive, StateStrictWWM, Worklist);
+        // Ignore these if V_SET_INACTIVE which already has exec src register.
+        // These are generated by an earlier pass which has seperately ensured
+        // WWM and provided a mask of inactive lanes.
+        Register ExecSrc = TII->findImplicitExecSrc(MI);
+        if (!ExecSrc) {
+          // Disable strict states; StrictWQM will be added as required later.
+          III.Disabled = StateStrict;
+          MachineOperand &Inactive = MI.getOperand(2);
+          if (Inactive.isReg()) {
+            if (Inactive.isUndef()) {
+              LowerToCopyInstrs.insert(&MI);
+            } else {
+              markOperand(MI, Inactive, StateStrictWWM, Worklist);
+            }
           }
+          SetInactiveInstrs.push_back(&MI);
+          BBI.NeedsLowering = true;
         }
-        SetInactiveInstrs.push_back(&MI);
-        BBI.NeedsLowering = true;
       } else if (TII->isDisableWQM(MI)) {
         BBI.Needs |= StateExact;
         if (!(BBI.InNeeds & StateExact)) {
diff --git a/llvm/test/CodeGen/AMDGPU/wqm.mir b/llvm/test/CodeGen/AMDGPU/wqm.mir
@@ -40,6 +40,9 @@
   define amdgpu_vs void @no_wqm_in_vs() {
     ret void
   }
+  define amdgpu_ps void @preloaded_set_inactive() {
+    ret void
+  }
 ...
 ---
 
@@ -443,3 +446,19 @@ body:             |
 
     %4:vreg_128 = IMAGE_SAMPLE_V4_V2 %0:vreg_64, %100:sgpr_256, %101:sgpr_128, 15, 0, 0, 0, 0, 0, 0, 0, implicit $exec :: (dereferenceable load (s128), align 4, addrspace 4)
 ...
+
+---
+# Preserve V_SET_INACTIVE with exec mask already specified
+#CHECK-LABEL: name: preloaded_set_inactive
+#CHECK: V_SET_INACTIVE_B32
+name:            preloaded_set_inactive
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $vgpr1, $vgpr2
+
+    %0:vgpr_32 = COPY $vgpr1
+    %1:vgpr_32 = COPY $vgpr2
+    %mask:sreg_64 = S_OR_SAVEEXEC_B64 -1, implicit-def $exec, implicit-def dead $scc, implicit $exec
+    %value:vgpr_32 = V_SET_INACTIVE_B32 %0:vgpr_32, %1:vgpr_32, implicit $exec, implicit-def $scc, implicit %mask:sreg_64
+...
