AMDGPU: Do not clobber SCC in SIWholeQuadMode

[arsenm]

Is WQM needed for correctness or is it just an optimization?

lib/Target/AMDGPU/SIWholeQuadMode.cpp

601

Should set DebugLoc

605–606

These can be combined with findRegisterUseOperandIdx

620

Should set DebugLoc

646

This function is kind of complicated to special case track SCC liveness. Can you use LivePhysRegs or check the LiveIntervals to simplify it (I think there was some issue with tracking physical registers with LIS last time I tried to do it, but I don't remember)

656

Capitalize

661–664

The Clean = true can be hoisted to the initializer

667–668

range loop

669

Maybe this should get the read operand and see if it is undef?

673–674

Should check if it's dead?

692

I think these should use MI's DebugLoc

700–701

Insert through LIS

704

This can just return Restore

test/CodeGen/AMDGPU/wqm.ll

423

I don't see check lines for the cmp + select restore pattern here. Where is the scc def? This test also probably needs a comment

429

CHECK-NEXT for s_cbranch_scc?

[arsenm]

Can you post an MI dump example of when this happens? It seems like this isn't something you should need to handle

[nhaehnle]

Rewrite in terms of LiveIntervals. (LivePhysRegs complains about remaining
virtual registers.)

Not sure about those DebugLocs. I'm not very familiar with their semantics,
but there isn't usually one particular instruction that can be pinned down
as the reason for inserting a new instruction (for example, when a new
instruction is added at the end of a basic block).

[nhaehnle]

WQM is needed for correctness of pixel shaders. It must be enabled for all calculations that feed into derivative calculations (explicit ones via DS and implicit ones via image sample instructions), and it must be disabled for all stores.

test/CodeGen/AMDGPU/wqm.ll

423

That's because I didn't actually manage to concoct a test where the pattern is unavoidable. Usually, the SCC def ends up right before the SCC use, so that the WQM instruction can just be moved around it.

I've tested the pattern with some artificial hacks in the code (in the latest version by setting First == Last in prepareInsertion).

The problem is that I cannot prove that it will never be needed. For example, it might be needed if the machine scheduler makes unusual decisions about moving store instructions between the SCC def and the SCC use.

[nhaehnle]

In D22198#482885, @arsenm wrote:

Can you post an MI dump example of when this happens? It seems like this isn't something you should need to handle

The added test case in an example (I actually encountered this out in the real world in a piglit test.)

As you can see from the excerpt at http://paste.ubuntu.com/19764805/, the most natural place to insert the s_wqm_b64 would be between the s_cmp_lt and the s_cbranch. What the new code does instead is to maintain a range of places where the s_wqm_b64 could be inserted (based on the correctness constraints for WQM), and then finds a place in that range where SCC isnt' live.

[nhaehnle]

Subsequent work revealed that there can be inconsistencies with SCC, so fix
those.

[arsenm]

include/llvm/CodeGen/LiveIntervalAnalysis.h

395–400

This should probably be a separate patch

lib/Target/AMDGPU/SIWholeQuadMode.cpp

531–535

I think this should use -1 instead of 1, and use compare != 0 to be more canonical

531–535

Maybe you should just emit COPY and move this expansion to copyPhysReg for scc

[nhaehnle]

• rebase
• use COPY from/to SCC for save
• separate removeRegUnit into its own patch

[nhaehnle]

It turns out that r280589 introduced a piglit regression that needs this
change to be applied already now rather than as part of later WQM rework.

I had originally planned this after D22195, but with the recent changes to
control flow lowering that other patch is currently not ready to land / be
adapted, so I adjusted this SCC fix patch so that it applies on current SVN
trunk.

[arsenm]

lib/Target/AMDGPU/SIWholeQuadMode.cpp

510

MI.isTransient() might be more accurate. INLINEASM for example will not be removed

[nhaehnle]

Use MI.isTransient()

[arsenm]

LGTM

include/llvm/CodeGen/LiveIntervalAnalysis.h

public:
     LiveRange *getCachedRegUnit(unsigned Unit) {
       return RegUnitRanges[Unit];
     }
 
     const LiveRange *getCachedRegUnit(unsigned Unit) const {
       return RegUnitRanges[Unit];
     }
 
+    /// removeRegUnit - Remove computed live range for Unit. Subsequent uses
+    /// should rely on on-demand recomputation.
+    void removeRegUnit(unsigned Unit) {
+      delete RegUnitRanges[Unit];
+      RegUnitRanges[Unit] = nullptr;
+    }
+
     /// Remove value numbers and related live segments starting at position
     /// @p Pos that are part of any liverange of physical register @p Reg or one
     /// of its subregisters.
     void removePhysRegDefAt(unsigned Reg, SlotIndex Pos);
 
     /// Remove value number and related live segments of @p LI and its subranges
     /// that start at position @p Pos.
     void removeVRegDefAt(LiveInterval &LI, SlotIndex Pos);

lib/Target/AMDGPU/SIWholeQuadMode.cpp

private:
   void propagateValueSub(const WorkItem::Value &V, LaneBitmask LaneMask,
                          const LiveRange &LR, std::vector<WorkItem> &Worklist);
   void propagateValue(const WorkItem::Value &V,
                       std::vector<WorkItem> &Worklist);
   void propagateInstruction(MachineInstr &MI, std::vector<WorkItem> &Worklist);
   void propagateBlock(MachineBasicBlock &MBB, std::vector<WorkItem> &Worklist);
   char analyzeFunction(MachineFunction &MF);
 
+  bool requiresCorrectState(const MachineInstr &MI) const;
+
+  MachineBasicBlock::iterator saveSCC(MachineBasicBlock &MBB,
+                                      MachineBasicBlock::iterator Before);
+  MachineBasicBlock::iterator prepareInsertion(MachineBasicBlock &MBB,
+                                               MachineBasicBlock::iterator First,
+                                               MachineBasicBlock::iterator Last,
+                                               bool PreferLast, bool SaveSCC);
   void toExact(MachineBasicBlock &MBB, MachineBasicBlock::iterator Before,
                unsigned SaveWQM, unsigned LiveMaskReg);
   void toWQM(MachineBasicBlock &MBB, MachineBasicBlock::iterator Before,
              unsigned SavedWQM);
   void processBlock(MachineBasicBlock &MBB, unsigned LiveMaskReg, bool isEntry);
 
   void lowerLiveMaskQueries(unsigned LiveMaskReg);
 
else if (WI.MBB)
       propagateBlock(*WI.MBB, Worklist);
     else
       propagateValue(WI.V, Worklist);
   }
 
   return GlobalFlags;
 }
 
+/// Whether \p MI really requires the exec state computed during analysis.
+///
+/// Scalar instructions must occasionally be marked WQM for correct propagation
+/// (e.g. thread masks leading up to branches), but when it comes actual
+/// execution, they don't care about EXEC.
+bool SIWholeQuadMode::requiresCorrectState(const MachineInstr &MI) const {
+  // Skip instructions that are not affected by EXEC
+  if (TII->isScalarUnit(MI))
+    return false;
+
+  // Generic instructions such as COPY will either disappear by register
+  // coalescing or be lowered to SALU or VALU instructions.
+  if (TargetInstrInfo::isGenericOpcode(MI.getOpcode())) {
+    if (MI.getNumExplicitOperands() >= 1) {
+      const MachineOperand &Op = MI.getOperand(0);
+      if (Op.isReg()) {
+        if (TRI->isSGPRReg(*MRI, Op.getReg())) {
+          // SGPR instructions are not affected by EXEC
+          return false;
+        }
+      }
+    }
+  }
+
+  return true;
+}
+
+
+MachineBasicBlock::iterator
+SIWholeQuadMode::saveSCC(MachineBasicBlock &MBB,
+                         MachineBasicBlock::iterator Before) {
+  unsigned SaveReg = MRI->createVirtualRegister(&AMDGPU::SReg_32RegClass);
+
+  MachineInstr *Save =
+      BuildMI(MBB, Before , DebugLoc(), TII->get(AMDGPU::S_CSELECT_B32),
+              SaveReg)
+          .addImm(1)
+          .addImm(0);
+  MachineInstr *Restore =
+      BuildMI(MBB, Before, DebugLoc(), TII->get(AMDGPU::S_CMP_GT_U32))
+          .addReg(SaveReg)
+          .addImm(0);
+
+  LIS->InsertMachineInstrInMaps(*Save);
+  LIS->InsertMachineInstrInMaps(*Restore);
+  LIS->createAndComputeVirtRegInterval(SaveReg);
+
+  return Restore;
+}
+
+MachineBasicBlock::iterator
+SIWholeQuadMode::prepareInsertion(MachineBasicBlock &MBB,
+                                  MachineBasicBlock::iterator First,
+                                  MachineBasicBlock::iterator Last,
+                                  bool PreferLast, bool SaveSCC) {
+  if (!SaveSCC)
+    return PreferLast ? Last : First;
+
+  LiveRange &LR = LIS->getRegUnit(*MCRegUnitIterator(AMDGPU::SCC, TRI));
+  auto MBBE = MBB.end();
+  SlotIndex FirstIdx = First != MBBE ? LIS->getInstructionIndex(*First)
+                                     : LIS->getMBBEndIdx(&MBB);
+  SlotIndex LastIdx = Last != MBBE ? LIS->getInstructionIndex(*Last)
+                                   : LIS->getMBBEndIdx(&MBB);
+  SlotIndex Idx = PreferLast ? LastIdx : FirstIdx;
+  const LiveRange::Segment *S;
+
+  for (;;) {
+    S = LR.getSegmentContaining(Idx);
+    if (!S)
+      break;
+
+    if (PreferLast) {
+      SlotIndex Next = S->start.getBaseIndex();
+      if (Next < FirstIdx)
+        break;
+      Idx = Next;
+    } else {
+      SlotIndex Next = S->end.getNextIndex().getBaseIndex();
+      if (Next > LastIdx)
+        break;
+      Idx = Next;
+    }
+  }
+
+  MachineBasicBlock::iterator MBBI;
+
+  if (MachineInstr *MI = LIS->getInstructionFromIndex(Idx))
+    MBBI = MI;
+  else {
+    assert(Idx == LIS->getMBBEndIdx(&MBB));
+    MBBI = MBB.end();
+  }
+
+  if (S)
+    MBBI = saveSCC(MBB, MBBI);
+
+  return MBBI;
+}
+
 void SIWholeQuadMode::toExact(MachineBasicBlock &MBB,
                               MachineBasicBlock::iterator Before,
                               unsigned SaveWQM, unsigned LiveMaskReg) {
   MachineInstr *MI;
 
   if (SaveWQM) {
     MI = BuildMI(MBB, Before, DebugLoc(), TII->get(AMDGPU::S_AND_SAVEEXEC_B64),
                  SaveWQM)
              .addReg(LiveMaskReg);
   } else {
     MI = BuildMI(MBB, Before, DebugLoc(), TII->get(AMDGPU::S_AND_B64),
                  AMDGPU::EXEC)
              .addReg(AMDGPU::EXEC)
              .addReg(LiveMaskReg);
   }
 
   LIS->InsertMachineInstrInMaps(*MI);
 }
 
 void SIWholeQuadMode::toWQM(MachineBasicBlock &MBB,
                             MachineBasicBlock::iterator Before,
                             unsigned SavedWQM) {
   MachineInstr *MI;
 
   if (SavedWQM) {
     MI = BuildMI(MBB, Before, DebugLoc(), TII->get(AMDGPU::COPY), AMDGPU::EXEC)
              .addReg(SavedWQM);
   } else {
     MI = BuildMI(MBB, Before, DebugLoc(), TII->get(AMDGPU::S_WQM_B64),
                  AMDGPU::EXEC)
              .addReg(AMDGPU::EXEC);
   }
 
   LIS->InsertMachineInstrInMaps(*MI);
void SIWholeQuadMode::processBlock(MachineBasicBlock &MBB, unsigned LiveMaskReg,
 
   if (!(BI.Needs.In & StateWQM))
     return;
 
   // This is a non-entry block that is WQM throughout, so no need to do
   // anything.
   if (!isEntry && !(BI.Needs.Self & StateExact) && BI.Needs.Out != StateExact)
     return;
 
   DEBUG(dbgs() << "\nProcessing block BB#" << MBB.getNumber() << ":\n");
 
+  // First pass: Scan forward to find the ranges in which switching
+  // instructions are required.
   unsigned SavedWQMReg = 0;
   bool WQMFromExec = isEntry;
   char State = isEntry ? StateExact : StateWQM;
-  MachineInstr *FirstNonWQM = nullptr;
 
-  auto II = MBB.getFirstNonPHI(), IE = MBB.end();
-  while (II != IE) {
-    MachineInstr &MI = *II;
-    ++II;
+  auto II = MBB.getFirstNonPHI(), IE = MBB.getFirstTerminator();
+  if (isEntry)
+    ++II; // Skip the instruction that saves LiveMask
 
-    // Skip instructions that are not affected by EXEC
-    if (TII->isScalarUnit(MI) && !MI.isTerminator())
-      continue;
+  MachineBasicBlock::iterator First = IE;
+  for (;;) {
+    char Needs = 0;
+    char OutNeeds = 0;
 
-    // Generic instructions such as COPY will either disappear by register
-    // coalescing or be lowered to SALU or VALU instructions.
-    if (TargetInstrInfo::isGenericOpcode(MI.getOpcode())) {
-      if (MI.getNumExplicitOperands() >= 1) {
-        const MachineOperand &Op = MI.getOperand(0);
-        if (Op.isReg()) {
-          if (TRI->isSGPRReg(*MRI, Op.getReg())) {
-            // SGPR instructions are not affected by EXEC
-            continue;
-          }
+    if (First == IE)
+      First = II;
+
+    if (II != IE) {
+      MachineInstr &MI = *II;
+
+      if (requiresCorrectState(MI)) {
+        auto III = Instructions.find(&MI);
+        if (III != Instructions.end()) {
+          Needs = III->second.Needs;
+          OutNeeds = III->second.OutNeeds;
         }
       }
+    } else {
+      // End of basic block
+      if ((BI.Needs.Out & StateWQM) && State != StateWQM)
+        Needs = StateWQM;
+      else if (BI.Needs.Out == StateExact && State != StateExact)
+        Needs = StateExact;
     }
 
-    DEBUG(dbgs() << "  " << MI);
+    if (Needs) {
+      if (Needs != State) {
+        MachineBasicBlock::iterator Before =
+          prepareInsertion(MBB, First, II, Needs == StateWQM,
+                           Needs == StateExact || WQMFromExec);
 
-    char Needs = 0;
-    char OutNeeds = 0;
-    auto InstrInfoIt = Instructions.find(&MI);
-    if (InstrInfoIt != Instructions.end()) {
-      Needs = InstrInfoIt->second.Needs;
-      OutNeeds = InstrInfoIt->second.OutNeeds;
-    }
-
-    // Keep track of the first consecutive non-WQM instruction, so that we
-    // switch away from WQM as soon as possible, potentially saving a small
-    // bit of bandwidth on loads.
-    if (Needs == StateWQM)
-      FirstNonWQM = nullptr;
-    else if (!FirstNonWQM)
-      FirstNonWQM = &MI;
-
-    // State switching
-    if (Needs && State != Needs) {
         if (Needs == StateExact) {
-        assert(!SavedWQMReg);
-
           if (!WQMFromExec && (OutNeeds & StateWQM))
             SavedWQMReg = MRI->createVirtualRegister(&AMDGPU::SReg_64RegClass);
 
-        toExact(MBB, FirstNonWQM, SavedWQMReg, LiveMaskReg);
+          toExact(MBB, Before, SavedWQMReg, LiveMaskReg);
         } else {
           assert(WQMFromExec == (SavedWQMReg == 0));
-        toWQM(MBB, &MI, SavedWQMReg);
+
+          toWQM(MBB, Before, SavedWQMReg);
 
           if (SavedWQMReg) {
             LIS->createAndComputeVirtRegInterval(SavedWQMReg);
             SavedWQMReg = 0;
           }
         }
 
         State = Needs;
       }
-  }
 
-  if ((BI.Needs.Out & StateWQM) && State != StateWQM) {
-    assert(WQMFromExec == (SavedWQMReg == 0));
-    toWQM(MBB, MBB.end(), SavedWQMReg);
+      First = IE;
+    }
 
-    if (SavedWQMReg)
-      LIS->createAndComputeVirtRegInterval(SavedWQMReg);
-  } else if (BI.Needs.Out == StateExact && State != StateExact) {
-    toExact(MBB, FirstNonWQM ? MachineBasicBlock::iterator(FirstNonWQM)
-                             : MBB.getFirstTerminator(),
-            0, LiveMaskReg);
+    if (II == IE)
+      break;
+    ++II;
   }
 }
 
 void SIWholeQuadMode::lowerLiveMaskQueries(unsigned LiveMaskReg) {
   for (MachineInstr *MI : LiveMaskQueries) {
     const DebugLoc &DL = MI->getDebugLoc();
     unsigned Dest = MI->getOperand(0).getReg();
     MachineInstr *NewMI =
if (GlobalFlags != StateWQM) {
     for (auto &BII : Blocks)
       processBlock(const_cast<MachineBasicBlock &>(*BII.first), LiveMaskReg,
                    BII.first == &*MF.begin());
   }
 
   if (LiveMaskReg)
     LIS->createAndComputeVirtRegInterval(LiveMaskReg);
 
+  // Physical registers like SCC aren't tracked by default anyway, so just
+  // removing the ranges we computed is the simplest option for maintaining
+  // the analysis results.
+  LIS->removeRegUnit(*MCRegUnitIterator(AMDGPU::SCC, TRI));
+
   return true;
 }

test/CodeGen/AMDGPU/wqm.ll

end:
   %r = phi <4 x float> [ %r.if, %if ], [ %r.else, %else ]
 
   %gep = getelementptr float, float addrspace(1)* %ptr, i32 %idx
   store float 1.0, float addrspace(1)* %gep
 
   ret <4 x float> %r
 }
 
+; Test awareness that s_wqm_b64 clobbers SCC.
+;
+; CHECK-LABEL: {{^}}test_scc:
+; CHECK: s_mov_b64 [[ORIG:s\[[0-9]+:[0-9]+\]]], exec
+; CHECK: s_wqm_b64 exec, exec
+; CHECK: s_cmp_
+; CHECK-NEXT: s_cbranch_scc
+; CHECK: ; %if
+; CHECK: s_and_b64 exec, exec, [[ORIG]]
+; CHECK: image_sample
+; CHECK: ; %else
+; CHECK: s_and_b64 exec, exec, [[ORIG]]
+; CHECK: image_sample
+; CHECK: ; %end
+define amdgpu_ps <4 x float> @test_scc(float addrspace(1)* inreg %ptr, i32 inreg %sel, i32 %idx) #1 {
+main_body:
+  %cc = icmp sgt i32 %sel, 0
+  br i1 %cc, label %if, label %else
+
+if:
+  %r.if = call <4 x float> @llvm.SI.image.sample.i32(i32 0, <8 x i32> undef, <4 x i32> undef, i32 15, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0)
+  br label %end
+
+else:
+  %r.else = call <4 x float> @llvm.SI.image.sample.v2i32(<2 x i32> <i32 0, i32 1>, <8 x i32> undef, <4 x i32> undef, i32 15, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0)
+  br label %end
+
+end:
+  %r = phi <4 x float> [ %r.if, %if ], [ %r.else, %else ]
+
+  %gep = getelementptr float, float addrspace(1)* %ptr, i32 %idx
+  store float 1.0, float addrspace(1)* %gep
+
+  ret <4 x float> %r
+}
+
 declare void @llvm.amdgcn.image.store.v4i32(<4 x float>, <4 x i32>, <8 x i32>, i32, i1, i1, i1, i1) #1
 
 declare <4 x float> @llvm.amdgcn.image.load.v4i32(<4 x i32>, <8 x i32>, i32, i1, i1, i1, i1) #2
 
 declare <4 x float> @llvm.SI.image.sample.i32(i32, <8 x i32>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32) #3
 declare <4 x float> @llvm.SI.image.sample.v2i32(<2 x i32>, <8 x i32>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32) #3
 declare <4 x float> @llvm.SI.image.sample.v4i32(<4 x i32>, <8 x i32>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32) #3
 
 declare float @llvm.SI.fs.interp(i32, i32, i32, <2 x i32>) #3
 declare void @llvm.AMDGPU.kill(float)
 declare void @llvm.SI.export(i32, i32, i32, i32, i32, float, float, float, float)
 
 attributes #1 = { nounwind }
 attributes #2 = { nounwind readonly }
 attributes #3 = { nounwind readnone }
 attributes #4 = { "amdgpu-ps-wqm-outputs" }