Revert "Reland "RegisterCoalescer: Add implicit-def of super register when coalescing SUBREG_TO_REG" (#134408)"

This reverts commit bae8f13.

Some issues were found:
* #151768
* #151592
* #134408 (comment)
* #151888 (comment)

I'll revert this for the time being while I investigate.

Author: sdesmalen-arm

llvm/lib/CodeGen/RegisterCoalescer.cpp

@@ -306,12 +306,7 @@ class RegisterCoalescer : private LiveRangeEdit::Delegate {
   /// number if it is not zero. If DstReg is a physical register and the
   /// existing subregister number of the def / use being updated is not zero,
   /// make sure to set it to the correct physical subregister.
-  ///
-  /// If \p SubregToRegSrcInst is not empty, we are coalescing a
-  /// `DstReg = SUBREG_TO_REG SrcReg`, which should introduce an
-  /// implicit-def of DstReg on instructions that define SrcReg.
-  void updateRegDefsUses(Register SrcReg, Register DstReg, unsigned SubIdx,
-                         ArrayRef<MachineInstr *> SubregToRegSrcInst = {});
+  void updateRegDefsUses(Register SrcReg, Register DstReg, unsigned SubIdx);
 
   /// If the given machine operand reads only undefined lanes add an undef
   /// flag.
@@ -1448,7 +1443,6 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
 
   // CopyMI may have implicit operands, save them so that we can transfer them
   // over to the newly materialized instruction after CopyMI is removed.
-  LaneBitmask NewMIImplicitOpsMask;
   SmallVector<MachineOperand, 4> ImplicitOps;
   ImplicitOps.reserve(CopyMI->getNumOperands() -
                       CopyMI->getDesc().getNumOperands());
@@ -1463,9 +1457,6 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
               (MO.getSubReg() == 0 && MO.getReg() == DstOperand.getReg())) &&
              "unexpected implicit virtual register def");
       ImplicitOps.push_back(MO);
-      if (MO.isDef() && MO.getReg().isVirtual() &&
-          MRI->shouldTrackSubRegLiveness(DstReg))
-        NewMIImplicitOpsMask |= MRI->getMaxLaneMaskForVReg(MO.getReg());
     }
   }
 
@@ -1508,11 +1499,14 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
       } else {
         assert(MO.getReg() == NewMI.getOperand(0).getReg());
 
-        // If lanemasks need to be tracked, compile the lanemask of the NewMI
-        // implicit def operands to avoid subranges for the super-regs from
-        // being removed by code later on in this function.
-        if (MRI->shouldTrackSubRegLiveness(MO.getReg()))
-          NewMIImplicitOpsMask |= MRI->getMaxLaneMaskForVReg(MO.getReg());
+        // We're only expecting another def of the main output, so the range
+        // should get updated with the regular output range.
+        //
+        // FIXME: The range updating below probably needs updating to look at
+        // the super register if subranges are tracked.
+        assert(!MRI->shouldTrackSubRegLiveness(DstReg) &&
+               "subrange update for implicit-def of super register may not be "
+               "properly handled");
       }
     }
   }
@@ -1612,8 +1606,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
           CurrIdx.getRegSlot(NewMI.getOperand(0).isEarlyClobber());
       VNInfo::Allocator &Alloc = LIS->getVNInfoAllocator();
       for (LiveInterval::SubRange &SR : DstInt.subranges()) {
-        if ((SR.LaneMask & DstMask).none() &&
-            (SR.LaneMask & NewMIImplicitOpsMask).none()) {
+        if ((SR.LaneMask & DstMask).none()) {
           LLVM_DEBUG(dbgs()
                      << "Removing undefined SubRange "
                      << PrintLaneMask(SR.LaneMask) << " : " << SR << "\n");
@@ -1877,14 +1870,11 @@ void RegisterCoalescer::addUndefFlag(const LiveInterval &Int, SlotIndex UseIdx,
   }
 }
 
-void RegisterCoalescer::updateRegDefsUses(
-    Register SrcReg, Register DstReg, unsigned SubIdx,
-    ArrayRef<MachineInstr *> SubregToRegSrcInsts) {
+void RegisterCoalescer::updateRegDefsUses(Register SrcReg, Register DstReg,
+                                          unsigned SubIdx) {
   bool DstIsPhys = DstReg.isPhysical();
   LiveInterval *DstInt = DstIsPhys ? nullptr : &LIS->getInterval(DstReg);
 
-  // Coalescing a COPY may expose reads of 'undef' subregisters.
-  // If so, then explicitly propagate 'undef' to those operands.
   if (DstInt && DstInt->hasSubRanges() && DstReg != SrcReg) {
     for (MachineOperand &MO : MRI->reg_operands(DstReg)) {
       if (MO.isUndef())
@@ -1901,15 +1891,6 @@ void RegisterCoalescer::updateRegDefsUses(
     }
   }
 
-  // If DstInt already has a subrange for the unused lanes, then we shouldn't
-  // create duplicate subranges when we update the interval for unused lanes.
-  LaneBitmask DstIntLaneMask;
-  if (DstInt && MRI->shouldTrackSubRegLiveness(DstReg)) {
-    for (LiveInterval::SubRange &SR : DstInt->subranges())
-      DstIntLaneMask |= SR.LaneMask;
-  }
-
-  // Go through all instructions to replace uses of 'SrcReg' by 'DstReg'.
   SmallPtrSet<MachineInstr *, 8> Visited;
   for (MachineRegisterInfo::reg_instr_iterator I = MRI->reg_instr_begin(SrcReg),
                                                E = MRI->reg_instr_end();
@@ -1933,80 +1914,6 @@ void RegisterCoalescer::updateRegDefsUses(
     if (DstInt && !Reads && SubIdx && !UseMI->isDebugInstr())
       Reads = DstInt->liveAt(LIS->getInstructionIndex(*UseMI));
 
-    bool RequiresImplicitRedef = false;
-    if (!SubregToRegSrcInsts.empty()) {
-      // We can only add an implicit-def and undef if the sub registers match,
-      // e.g.
-      //  %0:gr32      = INSTX
-      //  %0.sub8:gr32 = INSTY           // top 24 bits of %0 still defined
-      //  %1:gr64      = SUBREG_TO_REG 0, %0, %subreg.sub32
-      //
-      // This cannot be transformed into:
-      //  %1.sub32:gr64      = INSTX
-      //  undef %1.sub8:gr64 = INSTY , implicit-def %1
-      //
-      // Because that would thrash the top 24 bits of %1.sub32.
-      if (is_contained(SubregToRegSrcInsts, UseMI) &&
-          all_of(UseMI->defs(),
-                 [&SubIdx, &SrcReg](const MachineOperand &MO) -> bool {
-                   if (MO.getReg() != SrcReg || !MO.getSubReg() || MO.isUndef())
-                     return true;
-                   return SubIdx == MO.getSubReg();
-                 })) {
-        // Add implicit-def of super-register to express that the whole
-        // register is defined by the instruction.
-        MachineInstrBuilder MIB(*MF, UseMI);
-        MIB.addReg(DstReg, RegState::ImplicitDefine);
-        RequiresImplicitRedef = true;
-      }
-
-      // If the coalesed instruction doesn't fully define the register, we need
-      // to preserve the original super register liveness for SUBREG_TO_REG.
-      //
-      // We pretended SUBREG_TO_REG was a regular copy for coalescing purposes,
-      // but it introduces liveness for other subregisters. Downstream users may
-      // have been relying on those bits, so we need to ensure their liveness is
-      // captured with a def of other lanes.
-      if (DstInt && MRI->shouldTrackSubRegLiveness(DstReg)) {
-        // First check if there is sufficient granularity in terms of subranges.
-        LaneBitmask DstMask = MRI->getMaxLaneMaskForVReg(DstInt->reg());
-        LaneBitmask UsedLanes = TRI->getSubRegIndexLaneMask(SubIdx);
-        LaneBitmask UnusedLanes = DstMask & ~UsedLanes;
-        if ((UnusedLanes & ~DstIntLaneMask).any()) {
-          BumpPtrAllocator &Allocator = LIS->getVNInfoAllocator();
-          DstInt->createSubRangeFrom(Allocator, UnusedLanes, *DstInt);
-          DstIntLaneMask |= UnusedLanes;
-        }
-
-        // After duplicating the live ranges for the low/hi bits, we
-        // need to update the subranges of the DstReg interval such that
-        // for a case like this:
-        //
-        //       entry:
-        //  16B    %1:gpr32 = INSTRUCTION    (<=> UseMI)
-        //            :
-        //       if.then:
-        //  32B    %1:gpr32 = MOVIMM32 ..
-        //  48B    %0:gpr64 = SUBREG_TO_REG 0, %1, sub32
-        //
-        //  Only the MOVIMM32 require a def of the top lanes and any intervals
-        //  for the top 32-bits of the def at 16B should be removed.
-        for (LiveInterval::SubRange &SR : DstInt->subranges()) {
-          if (!Writes || RequiresImplicitRedef ||
-              (SR.LaneMask & UnusedLanes).none())
-            continue;
-
-          assert((SR.LaneMask & UnusedLanes) == SR.LaneMask &&
-                 "Unexpected lanemask. Subrange needs finer granularity");
-
-          SlotIndex UseIdx = LIS->getInstructionIndex(*UseMI).getRegSlot(false);
-          auto SegmentI = SR.find(UseIdx);
-          if (SegmentI != SR.end())
-            SR.removeSegment(SegmentI, true);
-        }
-      }
-    }
-
     // Replace SrcReg with DstReg in all UseMI operands.
     for (unsigned Op : Ops) {
       MachineOperand &MO = UseMI->getOperand(Op);
@@ -2015,7 +1922,7 @@ void RegisterCoalescer::updateRegDefsUses(
       // turn a full def into a read-modify-write sub-register def and vice
       // versa.
       if (SubIdx && MO.isDef())
-        MO.setIsUndef(!Reads || RequiresImplicitRedef);
+        MO.setIsUndef(!Reads);
 
       // A subreg use of a partially undef (super) register may be a complete
       // undef use now and then has to be marked that way.
@@ -2118,30 +2025,6 @@ void RegisterCoalescer::setUndefOnPrunedSubRegUses(LiveInterval &LI,
   LIS->shrinkToUses(&LI);
 }
 
-/// For a given use of value \p Idx, it returns the def in the current block,
-/// or otherwise all possible defs in preceding blocks.
-static bool FindDefInBlock(SmallPtrSetImpl<MachineBasicBlock *> &VisitedBlocks,
-                           SmallVector<MachineInstr *> &Instrs,
-                           LiveIntervals *LIS, LiveInterval &SrcInt,
-                           MachineBasicBlock *MBB, VNInfo *Idx) {
-  if (!Idx->isPHIDef()) {
-    MachineInstr *Def = LIS->getInstructionFromIndex(Idx->def);
-    assert(Def && "Unable to find a def for SUBREG_TO_REG source operand");
-    Instrs.push_back(Def);
-    return true;
-  }
-
-  bool Any = false;
-  if (VisitedBlocks.count(MBB))
-    return false;
-  VisitedBlocks.insert(MBB);
-  for (MachineBasicBlock *Pred : MBB->predecessors()) {
-    Any |= FindDefInBlock(VisitedBlocks, Instrs, LIS, SrcInt, Pred,
-                          SrcInt.getVNInfoBefore(LIS->getMBBEndIdx(Pred)));
-  }
-  return Any;
-}
-
 bool RegisterCoalescer::joinCopy(
     MachineInstr *CopyMI, bool &Again,
     SmallPtrSetImpl<MachineInstr *> &CurrentErasedInstrs) {
@@ -2273,35 +2156,6 @@ bool RegisterCoalescer::joinCopy(
     });
   }
 
-  SmallVector<MachineInstr *> SubregToRegSrcInsts;
-  if (CopyMI->isSubregToReg()) {
-    // For the case where the copy instruction is a SUBREG_TO_REG, e.g.
-    //
-    //   %0:gpr32 = movimm32 ..
-    //   %1:gpr64 = SUBREG_TO_REG 0, %0, sub32
-    //   ...
-    //   %0:gpr32 = COPY <something>
-    //
-    // After joining liveranges, the original `movimm32` will need an
-    // implicit-def to make it explicit that the entire register is written,
-    // i.e.
-    //
-    //   undef %0.sub32:gpr64 = movimm32 ..., implicit-def %0
-    //   ...
-    //   undef %0.sub32:gpr64 = COPY <something>  // Note that this does not
-    //                                            // require an implicit-def,
-    //                                            // because it has nothing to
-    //                                            // do with the SUBREG_TO_REG.
-    LiveInterval &SrcInt =
-        LIS->getInterval(CP.isFlipped() ? CP.getDstReg() : CP.getSrcReg());
-    SlotIndex SubregToRegSlotIdx = LIS->getInstructionIndex(*CopyMI);
-    SmallPtrSet<MachineBasicBlock *, 8> VisitedBlocks;
-    if (!FindDefInBlock(VisitedBlocks, SubregToRegSrcInsts, LIS, SrcInt,
-                        CopyMI->getParent(),
-                        SrcInt.Query(SubregToRegSlotIdx).valueIn()))
-      llvm_unreachable("SUBREG_TO_REG src requires a def");
-  }
-
   ShrinkMask = LaneBitmask::getNone();
   ShrinkMainRange = false;
 
@@ -2371,12 +2225,9 @@ bool RegisterCoalescer::joinCopy(
 
   // Rewrite all SrcReg operands to DstReg.
   // Also update DstReg operands to include DstIdx if it is set.
-  if (CP.getDstIdx()) {
-    assert(SubregToRegSrcInsts.empty() && "can this happen?");
+  if (CP.getDstIdx())
     updateRegDefsUses(CP.getDstReg(), CP.getDstReg(), CP.getDstIdx());
-  }
-  updateRegDefsUses(CP.getSrcReg(), CP.getDstReg(), CP.getSrcIdx(),
-                    SubregToRegSrcInsts);
+  updateRegDefsUses(CP.getSrcReg(), CP.getDstReg(), CP.getSrcIdx());
 
   // Shrink subregister ranges if necessary.
   if (ShrinkMask.any()) {