[AMDGPU] Return better Changed status from SIFoldOperands

[piotr]

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

1241

Looks tryConstantFoldOp may make changes?

[foad]

Account for tryConstantFoldOp making changes.

[foad]

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

1241

Wow, good catch! Should be fixed now. But I think we need some better testing for this kind of problem. I will see if D80916 can be extended to do hashing on MachineIR as well as IR.

[piotr]

LGTM

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

public:
   void foldOperand(MachineOperand &OpToFold,
                    MachineInstr *UseMI,
                    int UseOpIdx,
                    SmallVectorImpl<FoldCandidate> &FoldList,
                    SmallVectorImpl<MachineInstr *> &CopiesToReplace) const;
 
   bool tryFoldCndMask(MachineInstr &MI) const;
   bool tryFoldZeroHighBits(MachineInstr &MI) const;
-  void foldInstOperand(MachineInstr &MI, MachineOperand &OpToFold) const;
+  bool foldInstOperand(MachineInstr &MI, MachineOperand &OpToFold) const;
 
   const MachineOperand *isClamp(const MachineInstr &MI) const;
   bool tryFoldClamp(MachineInstr &MI);
 
   std::pair<const MachineOperand *, int> isOMod(const MachineInstr &MI) const;
   bool tryFoldOMod(MachineInstr &MI);
   bool tryFoldRegSequence(MachineInstr &MI);
   bool tryFoldLCSSAPhi(MachineInstr &MI);
if (ST->zeroesHigh16BitsOfDest(SrcDef->getOpcode())) {
     MRI->replaceRegWith(Dst, SrcDef->getOperand(0).getReg());
     MI.eraseFromParent();
     return true;
   }
 
   return false;
 }
 
-void SIFoldOperands::foldInstOperand(MachineInstr &MI,
+bool SIFoldOperands::foldInstOperand(MachineInstr &MI,
                                      MachineOperand &OpToFold) const {
   // We need mutate the operands of new mov instructions to add implicit
   // uses of EXEC, but adding them invalidates the use_iterator, so defer
   // this.
   SmallVector<MachineInstr *, 4> CopiesToReplace;
   SmallVector<FoldCandidate, 4> FoldList;
   MachineOperand &Dst = MI.getOperand(0);
+  bool Changed = false;
 
   if (OpToFold.isImm()) {
     for (auto &UseMI :
          make_early_inc_range(MRI->use_nodbg_instructions(Dst.getReg()))) {
       // Folding the immediate may reveal operations that can be constant
       // folded or replaced with a copy. This can happen for example after
       // frame indices are lowered to constants or from splitting 64-bit
       // constants.
       //
       // We may also encounter cases where one or both operands are
       // immediates materialized into a register, which would ordinarily not
       // be folded due to multiple uses or operand constraints.
-      if (tryConstantFoldOp(*MRI, TII, &UseMI))
+      if (tryConstantFoldOp(*MRI, TII, &UseMI)) {
         LLVM_DEBUG(dbgs() << "Constant folded " << UseMI);
+        Changed = true;
+      }
     }
   }
 
   bool FoldingImm = OpToFold.isImm() || OpToFold.isFI() || OpToFold.isGlobal();
   if (FoldingImm) {
     unsigned NumLiteralUses = 0;
     MachineOperand *NonInlineUse = nullptr;
     int NonInlineUseOpNo = -1;
if (FoldingImm) {
     for (auto U : UsesToProcess) {
       MachineInstr *UseMI = U->getParent();
 
       foldOperand(OpToFold, UseMI, UseMI->getOperandNo(U),
         FoldList, CopiesToReplace);
     }
   }
 
+  if (CopiesToReplace.empty() && FoldList.empty())
+    return Changed;
+
   MachineFunction *MF = MI.getParent()->getParent();
   // Make sure we add EXEC uses to any new v_mov instructions created.
   for (MachineInstr *Copy : CopiesToReplace)
     Copy->addImplicitDefUseOperands(*MF);
 
   for (FoldCandidate &Fold : FoldList) {
     assert(!Fold.isReg() || Fold.OpToFold);
     if (Fold.isReg() && Fold.OpToFold->getReg().isVirtual()) {
if (updateOperand(Fold, *TII, *TRI, *ST)) {
       LLVM_DEBUG(dbgs() << "Folded source from " << MI << " into OpNo "
                         << static_cast<int>(Fold.UseOpNo) << " of "
                         << *Fold.UseMI);
     } else if (Fold.isCommuted()) {
       // Restoring instruction's original operand order if fold has failed.
       TII->commuteInstruction(*Fold.UseMI, false);
     }
   }
+  return true;
 }
 
 // Clamp patterns are canonically selected to v_max_* instructions, so only
 // handle them.
 const MachineOperand *SIFoldOperands::isClamp(const MachineInstr &MI) const {
   unsigned Op = MI.getOpcode();
   switch (Op) {
   case AMDGPU::V_MAX_F32_e64:
bool SIFoldOperands::runOnMachineFunction(MachineFunction &MF) {
 
   // omod is ignored by hardware if IEEE bit is enabled. omod also does not
   // correctly handle signed zeros.
   //
   // FIXME: Also need to check strictfp
   bool IsIEEEMode = MFI->getMode().IEEE;
   bool HasNSZ = MFI->hasNoSignedZerosFPMath();
 
+  bool Changed = false;
   for (MachineBasicBlock *MBB : depth_first(&MF)) {
     MachineOperand *CurrentKnownM0Val = nullptr;
     for (auto &MI : make_early_inc_range(*MBB)) {
-      tryFoldCndMask(MI);
+      Changed |= tryFoldCndMask(MI);
 
-      if (tryFoldZeroHighBits(MI))
+      if (tryFoldZeroHighBits(MI)) {
+        Changed = true;
         continue;
+      }
 
-      if (MI.isRegSequence() && tryFoldRegSequence(MI))
+      if (MI.isRegSequence() && tryFoldRegSequence(MI)) {
+        Changed = true;
         continue;
+      }
 
-      if (MI.isPHI() && tryFoldLCSSAPhi(MI))
+      if (MI.isPHI() && tryFoldLCSSAPhi(MI)) {
+        Changed = true;
         continue;
+      }
 
-      if (MI.mayLoad() && tryFoldLoad(MI))
+      if (MI.mayLoad() && tryFoldLoad(MI)) {
+        Changed = true;
         continue;
+      }
 
       if (!TII->isFoldableCopy(MI)) {
         // Saw an unknown clobber of m0, so we no longer know what it is.
         if (CurrentKnownM0Val && MI.modifiesRegister(AMDGPU::M0, TRI))
           CurrentKnownM0Val = nullptr;
 
         // TODO: Omod might be OK if there is NSZ only on the source
         // instruction, and not the omod multiply.
         if (IsIEEEMode || (!HasNSZ && !MI.getFlag(MachineInstr::FmNsz)) ||
             !tryFoldOMod(MI))
-          tryFoldClamp(MI);
+          Changed |= tryFoldClamp(MI);
 
         continue;
       }
 
       // Specially track simple redefs of m0 to the same value in a block, so we
       // can erase the later ones.
       if (MI.getOperand(0).getReg() == AMDGPU::M0) {
         MachineOperand &NewM0Val = MI.getOperand(1);
         if (CurrentKnownM0Val && CurrentKnownM0Val->isIdenticalTo(NewM0Val)) {
           MI.eraseFromParent();
+          Changed = true;
           continue;
         }
 
         // We aren't tracking other physical registers
         CurrentKnownM0Val = (NewM0Val.isReg() && NewM0Val.getReg().isPhysical()) ?
           nullptr : &NewM0Val;
         continue;
       }
for (auto &MI : make_early_inc_range(*MBB)) {
       // the COPY opcode must not be replaced by 1 in this example:
       //
       //    %3 = COPY %vgpr0; VGPR_32:%3
       //    ...
       //    %vgpr0 = V_MOV_B32_e32 1, implicit %exec
       if (!MI.getOperand(0).getReg().isVirtual())
         continue;
 
-      foldInstOperand(MI, OpToFold);
+      Changed |= foldInstOperand(MI, OpToFold);
 
       // If we managed to fold all uses of this copy then we might as well
       // delete it now.
       // The only reason we need to follow chains of copies here is that
       // tryFoldRegSequence looks forward through copies before folding a
       // REG_SEQUENCE into its eventual users.
       auto *InstToErase = &MI;
       while (MRI->use_nodbg_empty(InstToErase->getOperand(0).getReg())) {
         auto &SrcOp = InstToErase->getOperand(1);
         auto SrcReg = SrcOp.isReg() ? SrcOp.getReg() : Register();
         InstToErase->eraseFromParent();
+        Changed = true;
         InstToErase = nullptr;
         if (!SrcReg || SrcReg.isPhysical())
           break;
         InstToErase = MRI->getVRegDef(SrcReg);
         if (!InstToErase || !TII->isFoldableCopy(*InstToErase))
           break;
       }
       if (InstToErase && InstToErase->isRegSequence() &&
-          MRI->use_nodbg_empty(InstToErase->getOperand(0).getReg()))
+          MRI->use_nodbg_empty(InstToErase->getOperand(0).getReg())) {
         InstToErase->eraseFromParent();
+        Changed = true;
       }
     }
-  return true;
+  }
+  return Changed;
 }