[AMDGPU] Fix trivial PHI into SI_END_CF.

[hliao]

Summary

• SI_END_CF, a MBB prologue instruction, should not take any source from PHI. But, LCSSA pass may insert one.
• Teach PHIElim to handle such case by providing target-specific hook to get the insert point for PHI lowering (replace PHI with COPY). So that, the target could canonicalize any MBB prologue instructions.

[arsenm]

llvm/lib/CodeGen/PHIElimination.cpp

203–204

Wait, we already have isBasicBlockPrologue, and SkipPHIsAndLabels already checks it. Is it not returning true for SI_END_CF for some reason?

[arsenm]

llvm/lib/CodeGen/PHIElimination.cpp

203–204

I think to fixup the prolog uses a phi case, no new hook is necessary and can be done in terms of the existing one

[hliao]

llvm/lib/CodeGen/PHIElimination.cpp

203–204

that hook sounds to me should not modify anything.

[nhaehnle]

How about the following simpler logic:

• if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
• otherwise, insert the COPY after the basic-block prologue

[rampitec]

In D64946#1593334, @nhaehnle wrote:

How about the following simpler logic:

• if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
• otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

[hliao]

It sounds to me that additional hook is a clear change. Or, we need to change the existing hook from const MachineInstr & to MachineInstr & to allow changing instruction. Any idea?

[nhaehnle]

In D64946#1593675, @rampitec wrote:

In D64946#1593334, @nhaehnle wrote:

How about the following simpler logic:

• if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
• otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

You're right, my idea doesn't work.

[rampitec]

In D64946#1595185, @nhaehnle wrote:

In D64946#1593675, @rampitec wrote:

In D64946#1593334, @nhaehnle wrote:

How about the following simpler logic:

• if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
• otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

You're right, my idea doesn't work.

Unless we find a way to mark it as a prologue. An easiest (but not clean) way is to mark copy as impdef exec.

[hliao]

when we insert that copy, if we still follow the current logic of phi-elim, that COPY is already inserted after its use (that CF_END).

In D64946#1595202, @rampitec wrote:

In D64946#1595185, @nhaehnle wrote:

In D64946#1593675, @rampitec wrote:

In D64946#1593334, @nhaehnle wrote:

How about the following simpler logic:

• if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
• otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

You're right, my idea doesn't work.

Unless we find a way to mark it as a prologue. An easiest (but not clean) way is to mark copy as impdef exec.

llvm/include/llvm/CodeGen/TargetInstrInfo.h

public:
 
   /// True if the instruction is bound to the top of its basic block and no
   /// other instructions shall be inserted before it. This can be implemented
   /// to prevent register allocator to insert spills before such instructions.
   virtual bool isBasicBlockPrologue(const MachineInstr &MI) const {
     return false;
   }
 
+  virtual MachineBasicBlock::iterator
+  getPHILoweringPoint(MachineBasicBlock &MBB) const {
+    return std::prev(MBB.SkipPHIsAndLabels(MBB.begin()));
+  }
+
   /// Returns a \p outliner::OutlinedFunction struct containing target-specific
   /// information for a set of outlining candidates.
   virtual outliner::OutlinedFunction getOutliningCandidateInfo(
       std::vector<outliner::Candidate> &RepeatedSequenceLocs) const {
     llvm_unreachable(
         "Target didn't implement TargetInstrInfo::getOutliningCandidateInfo!");
   }
 

llvm/lib/CodeGen/PHIElimination.cpp

 
 /// EliminatePHINodes - Eliminate phi nodes by inserting copy instructions in
 /// predecessor basic blocks.
 bool PHIElimination::EliminatePHINodes(MachineFunction &MF,
                                        MachineBasicBlock &MBB) {
   if (MBB.empty() || !MBB.front().isPHI())
     return false;   // Quick exit for basic blocks without PHIs.
 
+  const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
   // Get an iterator to the last PHI node.
-  MachineBasicBlock::iterator LastPHIIt =
-    std::prev(MBB.SkipPHIsAndLabels(MBB.begin()));
-
+  MachineBasicBlock::iterator LastPHIIt = TII->getPHILoweringPoint(MBB);
   while (MBB.front().isPHI())
     LowerPHINode(MBB, LastPHIIt);
 
   return true;
 }
 
 /// Return true if all defs of VirtReg are implicit-defs.
 /// This includes registers with no defs.

llvm/lib/Target/AMDGPU/SIInstrInfo.h

public:
   CreateTargetPostRAHazardRecognizer(const InstrItineraryData *II,
                                  const ScheduleDAG *DAG) const override;
 
   ScheduleHazardRecognizer *
   CreateTargetPostRAHazardRecognizer(const MachineFunction &MF) const override;
 
   bool isBasicBlockPrologue(const MachineInstr &MI) const override;
 
+  MachineBasicBlock::iterator
+  getPHILoweringPoint(MachineBasicBlock &MBB) const override;
+
   /// Return a partially built integer add instruction without carry.
   /// Caller must add source operands.
   /// For pre-GFX9 it will generate unused carry destination operand.
   /// TODO: After GFX9 it should return a no-carry operation.
   MachineInstrBuilder getAddNoCarry(MachineBasicBlock &MBB,
                                     MachineBasicBlock::iterator I,
                                     const DebugLoc &DL,
                                     unsigned DestReg) const;

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

SIInstrInfo::getSerializableDirectMachineOperandTargetFlags() const {
   return makeArrayRef(TargetFlags);
 }
 
 bool SIInstrInfo::isBasicBlockPrologue(const MachineInstr &MI) const {
   return !MI.isTerminator() && MI.getOpcode() != AMDGPU::COPY &&
          MI.modifiesRegister(AMDGPU::EXEC, &RI);
 }
 
+MachineBasicBlock::iterator
+SIInstrInfo::getPHILoweringPoint(MachineBasicBlock &MBB) const {
+  // Canonicalize PHI usage in SI_END_CF.
+  auto MII = MBB.getFirstNonPHI();
+  if (MII != MBB.end() && MII->getOpcode() == AMDGPU::SI_END_CF) {
+    MachineRegisterInfo &MRI = MBB.getParent()->getRegInfo();
+    unsigned Reg = MII->getOperand(0).getReg();
+    assert(MII->getOperand(0).getSubReg() == 0 && "Unexpected subregister");
+    // Check whether it's defined from PHI.
+    auto Def = MRI.getVRegDef(Reg);
+    if (Def->isPHI()) {
+      // It should a trivial PHI. Such a PHI node may be inserted as the last
+      // LCSSA before isel. It's meanless and should be eliminated.
+      assert(Def->getNumOperands() == 3 && "Unexpected PHI fed into SI_END_CF");
+      // Eliminate this trivial PHI.
+      unsigned SrcReg = Def->getOperand(1).getReg();
+      assert(Def->getOperand(1).getSubReg() == 0 &&
+             MRI.constrainRegClass(SrcReg, MRI.getRegClass(Reg)) &&
+             "Unexpected source register");
+      MRI.replaceRegWith(Reg, SrcReg);
+      Def->eraseFromParent();
+    }
+  }
+  return TargetInstrInfo::getPHILoweringPoint(MBB);
+}
+
 MachineInstrBuilder
 SIInstrInfo::getAddNoCarry(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator I,
                            const DebugLoc &DL,
                            unsigned DestReg) const {
   if (ST.hasAddNoCarry())
     return BuildMI(MBB, I, DL, get(AMDGPU::V_ADD_U32_e64), DestReg);
 

llvm/test/CodeGen/AMDGPU/lcssa-optnone.ll

+; RUN: llc -verify-machineinstrs -march=amdgcn -O0 -o - %s | FileCheck %s
+
+; CHECK-LABEL: non_uniform_loop
+; CHECK: s_endpgm
+define amdgpu_kernel void @non_uniform_loop(float addrspace(1)* %array) {
+entry:
+  %w = tail call i32 @llvm.amdgcn.workitem.id.x()
+  br label %for.cond
+
+for.cond:
+  %i = phi i32 [0, %entry], [%i.next, %for.inc]
+  %cmp = icmp ult i32 %i, %w
+  br i1 %cmp, label %for.body, label %for.end
+
+for.body:
+  br label %for.inc
+
+for.inc:
+  %i.next = add i32 %i, 1
+  br label %for.cond
+
+for.end:
+  ret void
+}
+
+declare i32 @llvm.amdgcn.workitem.id.x()