[AMDGPU] Prevent VGPR copies from moving across the EXEC mask definitions

[alex-t]

Summary

This change uses adding implicit use of AMDGPU::EXEC register to avoid VGPR copies movement across the instructions defining EXEC.

[rampitec]

Please do not forget to add llvm-commits.

I personally do not object this change, let's hear other voices.

[arsenm]

lib/Target/AMDGPU/SIFoldOperands.cpp

502–508 ↗

(On Diff #206265)

Blindly stripping off any implicit_operands could be error prone. We do use other implicit operands on V_MOV_B32 for indirect register indexing

lib/Target/AMDGPU/SIInstrInfo.cpp

3923–3930 ↗

(On Diff #206265)

I don't see why this is necessary

[alex-t]

lib/Target/AMDGPU/SIFoldOperands.cpp

502–508 ↗

(On Diff #206265)

This is done just before calling MachineInstr::addImplicitUseDefOperands().
Also, this is under the "if" condition UseMI->isCopy() so not V_MOV exist so far.

lib/Target/AMDGPU/SIInstrInfo.cpp

3923–3930 ↗

(On Diff #206265)

To prevent moving VGPR copies across the EXEC definitions

[rampitec]

lib/Target/AMDGPU/SIFoldOperands.cpp

504 ↗

(On Diff #206265)

I believe the intent was to remove only duplicate operands, not all implicit operands at all.

[nhaehnle]

lib/Target/AMDGPU/SIInstrInfo.cpp

3923–3930 ↗

(On Diff #206265)

I think the question is more, why isn't the addOperand executed unconditionally for non-SGPR destinations? That is:

if (!RI.isSGPRClass(DstRC) && !Copy->readsRegister(AMDGPU::EXEC, &RI))
  Copy->addOperand(MachineOperand::CreateReg(AMDGPU::EXEC, false, true));

... without the preceding loop.

Presumably the answer might have to do with PHIs that have undef / IMPLICIT_DEF operands? Though, isn't it preferable for those to just be reduced to an IMPLICIT_DEF instead of a COPY from an IMPLICIT_DEF? And if the COPY is optimized away later, does adding the EXEC really hurt?

[alex-t]

lib/Target/AMDGPU/SIFoldOperands.cpp

504 ↗

(On Diff #206265)

For the AMDGPU::COPY, that is the only case here, we don't expect any implicit operands to exist except those added by legalizeGenericOperand.

lib/Target/AMDGPU/SIInstrInfo.cpp

3923–3930 ↗

(On Diff #206265)

The PHIs that have an input of COPY from IMPLICIT_DEF were exactly the reason for the "preceding loop" :)
Since the COPY from IMPLICIT_DEF to VGPR gets equipped with implicit use of EXEC it is not considered as the conversion candidate by the processImplicitDefs pass. So, I just excluded them from marking as using EXEC.

[arsenm]

I'm still not convinced just sticking EXEC reads on copies like this is a complete solution. Yes, we'll now insert the exec read on any copies that came out of isel, but any pass could introduce new copies. I guess if generic passes were more restricted when copies have implicit uses? It would require careful auditing of any generic code that can introduce copies, and may be more pessimizing than we really want

lib/Target/AMDGPU/SIFoldOperands.cpp

502–508 ↗

(On Diff #206265)

This would be a lot simpler to do
assert(UseMI->getNumOperands() == 3 && UseMI->getOperand(2) == exec), RemoveOperand(2).

I think this part should be split off into a separate patch to fix the extra implicit uses

[alex-t]

lib/Target/AMDGPU/SIFoldOperands.cpp

502–508 ↗

(On Diff #206265)

As you mentioned, there may be another copy makers besides the legalizeGenericOoperand.
They not necessarily have extra EXEC operand. I primarily take care of the copies introduced by the PHI legalization.

[alex-t]

Just ping! Does anybody has any objections?

[qcolombet]

lib/CodeGen/PeepholeOptimizer.cpp

1811 ↗

(On Diff #206265)

We should check that we don't have any implicit def and that would be good.
Generally speaking, I am somewhat uncomfortable that we attach thing to COPY, but as long as this is just implicit use, that's less critical. Otherwise, like the previous comment said, this will break in subtle way.

[alex-t]

According the reviewer's request the check for implicit defs has been added.
New helper functions in MachineInstr.h are necessary because the current interface like MachineInstr::getNumExplicitDefs returns just the MCInstDesc::NumDefs for non variadic opcodes.

[qcolombet]

LGTM for the generic part.

Nits below.

llvm/include/llvm/CodeGen/MachineInstr.h

430 ↗

(On Diff #214124)

Nit: Period at the end of sentence.

435 ↗

(On Diff #214124)

MO.isDef implies MO.isReg.
You can simplify the check.

448 ↗

(On Diff #214124)

Implicit operands must be registers.
Thus, you can simplify all this with:

return getNumOperands() - getNumExplicitOperands();

[alex-t]

Suggested nits added.

llvm/trunk/include/llvm/CodeGen/MachineInstr.h

MachineOperand& getOperand(unsigned i) {
     return Operands[i];
   }
 
   /// Returns the total number of definitions.
   unsigned getNumDefs() const {
     return getNumExplicitDefs() + MCID->getNumImplicitDefs();
   }
 
+  /// Returns true if the instruction has implicit definition.
+  bool hasImplicitDef() const {
+    for (unsigned I = getNumExplicitOperands(), E = getNumOperands();
+      I != E; ++I) {
+      const MachineOperand &MO = getOperand(I);
+      if (MO.isDef() && MO.isImplicit())
+        return true;
+    }
+    return false;
+  }
+
+  /// Returns the implicit operands number.
+  unsigned getNumImplicitOperands() const {
+    return getNumOperands() - getNumExplicitOperands();
+  }
+
   /// Return true if operand \p OpIdx is a subregister index.
   bool isOperandSubregIdx(unsigned OpIdx) const {
     assert(getOperand(OpIdx).getType() == MachineOperand::MO_Immediate &&
            "Expected MO_Immediate operand type.");
     if (isExtractSubreg() && OpIdx == 2)
       return true;
     if (isInsertSubreg() && OpIdx == 3)
       return true;

llvm/trunk/lib/CodeGen/PeepholeOptimizer.cpp

bool PeepholeOptimizer::runOnMachineFunction(MachineFunction &MF) {
 
   return Changed;
 }
 
 ValueTrackerResult ValueTracker::getNextSourceFromCopy() {
   assert(Def->isCopy() && "Invalid definition");
   // Copy instruction are supposed to be: Def = Src.
   // If someone breaks this assumption, bad things will happen everywhere.
-  assert(Def->getNumOperands() == 2 && "Invalid number of operands");
+  // There may be implicit uses preventing the copy to be moved across
+  // some target specific register definitions
+  assert(Def->getNumOperands() - Def->getNumImplicitOperands() == 2 &&
+         "Invalid number of operands");
+  assert(!Def->hasImplicitDef() && "Only implicit uses are allowed");
 
   if (Def->getOperand(DefIdx).getSubReg() != DefSubReg)
     // If we look for a different subreg, it means we want a subreg of src.
     // Bails as we do not support composing subregs yet.
     return ValueTrackerResult();
   // Otherwise, we want the whole source.
   const MachineOperand &Src = Def->getOperand(1);
   if (Src.isUndef())

llvm/trunk/lib/Target/AMDGPU/SIFoldOperands.cpp

if (FoldingImmLike && UseMI->isCopy()) {
     // In order to fold immediates into copies, we need to change the
     // copy to a MOV.
 
     unsigned MovOp = TII->getMovOpcode(DestRC);
     if (MovOp == AMDGPU::COPY)
       return;
 
     UseMI->setDesc(TII->get(MovOp));
+    MachineInstr::mop_iterator ImpOpI = UseMI->implicit_operands().begin();
+    MachineInstr::mop_iterator ImpOpE = UseMI->implicit_operands().end();
+    while (ImpOpI != ImpOpE) {
+      MachineInstr::mop_iterator Tmp = ImpOpI;
+      ImpOpI++;
+      UseMI->RemoveOperand(UseMI->getOperandNo(Tmp));
+    }
     CopiesToReplace.push_back(UseMI);
   } else {
     if (UseMI->isCopy() && OpToFold.isReg() &&
         Register::isVirtualRegister(UseMI->getOperand(0).getReg()) &&
         TRI->isVectorRegister(*MRI, UseMI->getOperand(0).getReg()) &&
         TRI->isVectorRegister(*MRI, UseMI->getOperand(1).getReg()) &&
         !UseMI->getOperand(1).getSubReg()) {
       unsigned Size = TII->getOpSize(*UseMI, 1);

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp

void SIInstrInfo::legalizeGenericOperand(MachineBasicBlock &InsertMBB,
 
   MachineInstr *Def = MRI.getVRegDef(OpReg);
   if (!Def)
     return;
 
   // Try to eliminate the copy if it is copying an immediate value.
   if (Def->isMoveImmediate())
     FoldImmediate(*Copy, *Def, OpReg, &MRI);
+
+  bool ImpDef = Def->isImplicitDef();
+  while (!ImpDef && Def && Def->isCopy()) {
+    Def = MRI.getUniqueVRegDef(Def->getOperand(1).getReg());
+    ImpDef = Def && Def->isImplicitDef();
+  }
+  if (!RI.isSGPRClass(DstRC) && !Copy->readsRegister(AMDGPU::EXEC, &RI) &&
+      !ImpDef)
+    Copy->addOperand(MachineOperand::CreateReg(AMDGPU::EXEC, false, true));
 }
 
 // Emit the actual waterfall loop, executing the wrapped instruction for each
 // unique value of \p Rsrc across all lanes. In the best case we execute 1
 // iteration, in the worst case we execute 64 (once per lane).
 static void
 emitLoadSRsrcFromVGPRLoop(const SIInstrInfo &TII, MachineRegisterInfo &MRI,
                           MachineBasicBlock &OrigBB, MachineBasicBlock &LoopBB,

llvm/trunk/test/CodeGen/AMDGPU/constant-fold-imm-immreg.mir

body:             |
   %2:vgpr_32 = V_LSHL_OR_B32 %0, %1, 10, implicit $exec
   S_ENDPGM 0, implicit %2
 
 ...
 
 ---
 
 # GCN-LABEL: name: constant_fold_lshl_or_reg0_immreg_immreg{{$}}
-# GCN: %3:vgpr_32 = V_MOV_B32_e32 12345, implicit $exec, implicit $exec
+# GCN: %3:vgpr_32 = V_MOV_B32_e32 12345, implicit $exec
 # GCN-NEXT: S_ENDPGM 0, implicit %3
 
 name: constant_fold_lshl_or_reg0_immreg_immreg
 alignment:       0
 exposesReturnsTwice: false
 legalized:       false
 regBankSelected: false
 selected:        false