PeepholeOptimizer: Handle undef operands correctly

[MatzeB]

Summary

The PeepholeOptimizer doesn't correctly propagate undef and would fail for vregs without definition.

Found while manually crafting testcases, in practice this is probably rare as it would mean incomplete DCE.

This changes the code to simply bail out if undef operands are involved (which is simpler code than correctly propagating the undef flag).

include/llvm/CodeGen/TargetInstrInfo.h

struct RegSubRegPairAndIdx : RegSubRegPair {
     RegSubRegPairAndIdx(unsigned Reg = 0, unsigned SubReg = 0,
                         unsigned SubIdx = 0)
         : RegSubRegPair(Reg, SubReg), SubIdx(SubIdx) {}
   };
 
   /// Build the equivalent inputs of a REG_SEQUENCE for the given \p MI
   /// and \p DefIdx.
   /// \p [out] InputRegs of the equivalent REG_SEQUENCE. Each element of
-  /// the list is modeled as <Reg:SubReg, SubIdx>.
+  /// the list is modeled as <Reg:SubReg, SubIdx>. Operands with the undef
+  /// flag are not added to this list.
   /// E.g., REG_SEQUENCE vreg1:sub1, sub0, vreg2, sub1 would produce
   /// two elements:
   /// - vreg1:sub1, sub0
   /// - vreg2<:0>, sub1
   ///
   /// \returns true if it is possible to build such an input sequence
   /// with the pair \p MI, \p DefIdx. False otherwise.
   ///
   /// \pre MI.isRegSequence() or MI.isRegSequenceLike().
   ///
   /// \note The generic implementation does not provide any support for
   /// MI.isRegSequenceLike(). In other words, one has to override
   /// getRegSequenceLikeInputs for target specific instructions.
   bool
   getRegSequenceInputs(const MachineInstr &MI, unsigned DefIdx,
                        SmallVectorImpl<RegSubRegPairAndIdx> &InputRegs) const;
 
   /// Build the equivalent inputs of a EXTRACT_SUBREG for the given \p MI
   /// and \p DefIdx.
   /// \p [out] InputReg of the equivalent EXTRACT_SUBREG.
   /// E.g., EXTRACT_SUBREG vreg1:sub1, sub0, sub1 would produce:
   /// - vreg1:sub1, sub0
   ///
   /// \returns true if it is possible to build such an input sequence
-  /// with the pair \p MI, \p DefIdx. False otherwise.
+  /// with the pair \p MI, \p DefIdx and the operand has no undef flag set.
+  /// False otherwise.
   ///
   /// \pre MI.isExtractSubreg() or MI.isExtractSubregLike().
   ///
   /// \note The generic implementation does not provide any support for
   /// MI.isExtractSubregLike(). In other words, one has to override
   /// getExtractSubregLikeInputs for target specific instructions.
   bool getExtractSubregInputs(const MachineInstr &MI, unsigned DefIdx,
                               RegSubRegPairAndIdx &InputReg) const;
 
   /// Build the equivalent inputs of a INSERT_SUBREG for the given \p MI
   /// and \p DefIdx.
   /// \p [out] BaseReg and \p [out] InsertedReg contain
   /// the equivalent inputs of INSERT_SUBREG.
   /// E.g., INSERT_SUBREG vreg0:sub0, vreg1:sub1, sub3 would produce:
   /// - BaseReg: vreg0:sub0
   /// - InsertedReg: vreg1:sub1, sub3
   ///
   /// \returns true if it is possible to build such an input sequence
-  /// with the pair \p MI, \p DefIdx. False otherwise.
+  /// with the pair \p MI, \p DefIdx and the operand has no undef flag set.
+  /// False otherwise.
   ///
   /// \pre MI.isInsertSubreg() or MI.isInsertSubregLike().
   ///
   /// \note The generic implementation does not provide any support for
   /// MI.isInsertSubregLike(). In other words, one has to override
   /// getInsertSubregLikeInputs for target specific instructions.
   bool getInsertSubregInputs(const MachineInstr &MI, unsigned DefIdx,
                              RegSubRegPair &BaseReg,

lib/CodeGen/PeepholeOptimizer.cpp

ValueTrackerResult ValueTracker::getNextSourceFromCopy() {
   assert(Def->getNumOperands() == 2 && "Invalid number of operands");
 
   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())
+    return ValueTrackerResult();
   return ValueTrackerResult(Src.getReg(), Src.getSubReg());
 }
 
 ValueTrackerResult ValueTracker::getNextSourceFromBitcast() {
   assert(Def->isBitcast() && "Invalid definition");
 
   // Bail if there are effects that a plain copy will not expose.
   if (Def->hasUnmodeledSideEffects())
ValueTrackerResult ValueTracker::getNextSourceFromBitcast() {
   // Stop when any user of the bitcast is a SUBREG_TO_REG, replacing with a COPY
   // will break the assumed guarantees for the upper bits.
   for (const MachineInstr &UseMI : MRI.use_nodbg_instructions(DefOp.getReg())) {
     if (UseMI.isSubregToReg())
       return ValueTrackerResult();
   }
 
   const MachineOperand &Src = Def->getOperand(SrcIdx);
+  if (Src.isUndef())
+    return ValueTrackerResult();
   return ValueTrackerResult(Src.getReg(), Src.getSubReg());
 }
 
 ValueTrackerResult ValueTracker::getNextSourceFromRegSequence() {
   assert((Def->isRegSequence() || Def->isRegSequenceLike()) &&
          "Invalid definition");
 
   if (Def->getOperand(DefIdx).getSubReg())
ValueTrackerResult ValueTracker::getNextSourceFromPHI() {
   // subregs yet.
   if (Def->getOperand(0).getSubReg() != DefSubReg)
     return ValueTrackerResult();
 
   // Return all register sources for PHI instructions.
   for (unsigned i = 1, e = Def->getNumOperands(); i < e; i += 2) {
     auto &MO = Def->getOperand(i);
     assert(MO.isReg() && "Invalid PHI instruction");
+    // We have no code to deal with undef operands. They shouldn't happen in
+    // normal programs anyway.
+    if (MO.isUndef())
+      return ValueTrackerResult();
     Res.addSource(MO.getReg(), MO.getSubReg());
   }
 
   return Res;
 }
 
 ValueTrackerResult ValueTracker::getNextSourceImpl() {
   assert(Def && "This method needs a valid definition");
if (OneRegSrc)
       Reg = Res.getSrcReg(0);
     // Update the result before moving up in the use-def chain
     // with the instruction containing the last found sources.
     Res.setInst(Def);
 
     // If we can still move up in the use-def chain, move to the next
     // definition.
     if (!TargetRegisterInfo::isPhysicalRegister(Reg) && OneRegSrc) {
-      Def = MRI.getVRegDef(Reg);
-      DefIdx = MRI.def_begin(Reg).getOperandNo();
+      MachineRegisterInfo::def_iterator DI = MRI.def_begin(Reg);
+      if (DI != MRI.def_end()) {
+        Def = DI->getParent();
+        DefIdx = DI.getOperandNo();
         DefSubReg = Res.getSrcSubReg(0);
+      } else {
+        Def = nullptr;
+      }
       return Res;
     }
   }
   // If we end up here, this means we will not be able to find another source
   // for the next iteration. Make sure any new call to getNextSource bails out
   // early by cutting the use-def chain.
   Def = nullptr;
   return Res;
 }

lib/CodeGen/TargetInstrInfo.cpp

if (!MI.isRegSequence())
     return getRegSequenceLikeInputs(MI, DefIdx, InputRegs);
 
   // We are looking at:
   // Def = REG_SEQUENCE v0, sub0, v1, sub1, ...
   assert(DefIdx == 0 && "REG_SEQUENCE only has one def");
   for (unsigned OpIdx = 1, EndOpIdx = MI.getNumOperands(); OpIdx != EndOpIdx;
        OpIdx += 2) {
     const MachineOperand &MOReg = MI.getOperand(OpIdx);
+    if (MOReg.isUndef())
+      continue;
     const MachineOperand &MOSubIdx = MI.getOperand(OpIdx + 1);
     assert(MOSubIdx.isImm() &&
            "One of the subindex of the reg_sequence is not an immediate");
     // Record Reg:SubReg, SubIdx.
     InputRegs.push_back(RegSubRegPairAndIdx(MOReg.getReg(), MOReg.getSubReg(),
                                             (unsigned)MOSubIdx.getImm()));
   }
   return true;
 }
 
 bool TargetInstrInfo::getExtractSubregInputs(
     const MachineInstr &MI, unsigned DefIdx,
     RegSubRegPairAndIdx &InputReg) const {
   assert((MI.isExtractSubreg() ||
       MI.isExtractSubregLike()) && "Instruction do not have the proper type");
 
   if (!MI.isExtractSubreg())
     return getExtractSubregLikeInputs(MI, DefIdx, InputReg);
 
   // We are looking at:
   // Def = EXTRACT_SUBREG v0.sub1, sub0.
   assert(DefIdx == 0 && "EXTRACT_SUBREG only has one def");
   const MachineOperand &MOReg = MI.getOperand(1);
+  if (MOReg.isUndef())
+    return false;
   const MachineOperand &MOSubIdx = MI.getOperand(2);
   assert(MOSubIdx.isImm() &&
          "The subindex of the extract_subreg is not an immediate");
 
   InputReg.Reg = MOReg.getReg();
   InputReg.SubReg = MOReg.getSubReg();
   InputReg.SubIdx = (unsigned)MOSubIdx.getImm();
   return true;
 }
 
 bool TargetInstrInfo::getInsertSubregInputs(
     const MachineInstr &MI, unsigned DefIdx,
     RegSubRegPair &BaseReg, RegSubRegPairAndIdx &InsertedReg) const {
   assert((MI.isInsertSubreg() ||
       MI.isInsertSubregLike()) && "Instruction do not have the proper type");
 
   if (!MI.isInsertSubreg())
     return getInsertSubregLikeInputs(MI, DefIdx, BaseReg, InsertedReg);
 
   // We are looking at:
   // Def = INSERT_SEQUENCE v0, v1, sub0.
   assert(DefIdx == 0 && "INSERT_SUBREG only has one def");
   const MachineOperand &MOBaseReg = MI.getOperand(1);
   const MachineOperand &MOInsertedReg = MI.getOperand(2);
+  if (MOInsertedReg.isUndef())
+    return false;
   const MachineOperand &MOSubIdx = MI.getOperand(3);
   assert(MOSubIdx.isImm() &&
          "One of the subindex of the reg_sequence is not an immediate");
   BaseReg.Reg = MOBaseReg.getReg();
   BaseReg.SubReg = MOBaseReg.getSubReg();
 
   InsertedReg.Reg = MOInsertedReg.getReg();
   InsertedReg.SubReg = MOInsertedReg.getSubReg();
   InsertedReg.SubIdx = (unsigned)MOSubIdx.getImm();
   return true;
 }

lib/Target/ARM/ARMBaseInstrInfo.cpp

bool ARMBaseInstrInfo::getRegSequenceLikeInputs(
   switch (MI.getOpcode()) {
   case ARM::VMOVDRR:
     // dX = VMOVDRR rY, rZ
     // is the same as:
     // dX = REG_SEQUENCE rY, ssub_0, rZ, ssub_1
     // Populate the InputRegs accordingly.
     // rY
     const MachineOperand *MOReg = &MI.getOperand(1);
-    InputRegs.push_back(
-        RegSubRegPairAndIdx(MOReg->getReg(), MOReg->getSubReg(), ARM::ssub_0));
+    if (!MOReg->isUndef())
+      InputRegs.push_back(RegSubRegPairAndIdx(MOReg->getReg(),
+                                              MOReg->getSubReg(), ARM::ssub_0));
     // rZ
     MOReg = &MI.getOperand(2);
-    InputRegs.push_back(
-        RegSubRegPairAndIdx(MOReg->getReg(), MOReg->getSubReg(), ARM::ssub_1));
+    if (!MOReg->isUndef())
+      InputRegs.push_back(RegSubRegPairAndIdx(MOReg->getReg(),
+                                              MOReg->getSubReg(), ARM::ssub_1));
     return true;
   }
   llvm_unreachable("Target dependent opcode missing");
 }
 
 bool ARMBaseInstrInfo::getExtractSubregLikeInputs(
     const MachineInstr &MI, unsigned DefIdx,
     RegSubRegPairAndIdx &InputReg) const {
   assert(DefIdx < MI.getDesc().getNumDefs() && "Invalid definition index");
   assert(MI.isExtractSubregLike() && "Invalid kind of instruction");
 
   switch (MI.getOpcode()) {
   case ARM::VMOVRRD:
     // rX, rY = VMOVRRD dZ
     // is the same as:
     // rX = EXTRACT_SUBREG dZ, ssub_0
     // rY = EXTRACT_SUBREG dZ, ssub_1
     const MachineOperand &MOReg = MI.getOperand(2);
+    if (MOReg.isUndef())
+      return false;
     InputReg.Reg = MOReg.getReg();
     InputReg.SubReg = MOReg.getSubReg();
     InputReg.SubIdx = DefIdx == 0 ? ARM::ssub_0 : ARM::ssub_1;
     return true;
   }
   llvm_unreachable("Target dependent opcode missing");
 }
 
 bool ARMBaseInstrInfo::getInsertSubregLikeInputs(
     const MachineInstr &MI, unsigned DefIdx, RegSubRegPair &BaseReg,
     RegSubRegPairAndIdx &InsertedReg) const {
   assert(DefIdx < MI.getDesc().getNumDefs() && "Invalid definition index");
   assert(MI.isInsertSubregLike() && "Invalid kind of instruction");
 
   switch (MI.getOpcode()) {
   case ARM::VSETLNi32:
     // dX = VSETLNi32 dY, rZ, imm
     const MachineOperand &MOBaseReg = MI.getOperand(1);
     const MachineOperand &MOInsertedReg = MI.getOperand(2);
+    if (MOInsertedReg.isUndef())
+      return false;
     const MachineOperand &MOIndex = MI.getOperand(3);
     BaseReg.Reg = MOBaseReg.getReg();
     BaseReg.SubReg = MOBaseReg.getSubReg();
 
     InsertedReg.Reg = MOInsertedReg.getReg();
     InsertedReg.SubReg = MOInsertedReg.getSubReg();
     InsertedReg.SubIdx = MOIndex.getImm() == 0 ? ARM::ssub_0 : ARM::ssub_1;
     return true;
   }
   llvm_unreachable("Target dependent opcode missing");
 }

test/CodeGen/ARM/peephole-phi.mir

body: |
   bb.2:
     %3:spr = IMPLICIT_DEF
     %2:gpr = VMOVRS %3:spr, 14, _
 
   bb.3:
     %4:gpr = PHI %0, %bb.1, %2, %bb.2
     %5:spr = VMOVSR %4, 14, _
 ...
+
+# The current implementation doesn't perform any transformations if undef
+# operands are involved.
+# CHECK-LABEL: name: func-undefops
+# CHECK: body: |
+# CHECK:   bb.0:
+# CHECK:     Bcc %bb.2, 1, undef %cpsr
+#
+# CHECK:   bb.1:
+# CHECK:     %0:gpr = VMOVRS undef %1:spr, 14, _
+# CHECK:     B %bb.3
+#
+# CHECK:   bb.2:
+# CHECK:     %2:gpr = VMOVRS undef %3:spr, 14, _
+#
+# CHECK:   bb.3:
+# CHECK:     %4:gpr = PHI %0, %bb.1, %2, %bb.2
+# CHECK:     %5:spr = VMOVSR %4, 14, _
+---
+name: func-undefops
+tracksRegLiveness: true
+body: |
+  bb.0:
+    Bcc %bb.2, 1, undef %cpsr
+
+  bb.1:
+    %0:gpr = VMOVRS undef %1:spr, 14, _
+    B %bb.3
+
+  bb.2:
+    %2:gpr = VMOVRS undef %3:spr, 14, _
+
+  bb.3:
+    %4:gpr = PHI %0, %bb.1, %2, %bb.2
+    %5:spr = VMOVSR %4, 14, _
+...