[llvm] [1/2][AMDGPU] Fixed crash due to virtual register defs not dominating uses (PR #198472)

[via llvm-commits]

================
@@ -476,6 +492,69 @@ void AMDGPURewriteAGPRCopyMFMAImpl::collectSpillIndexUses(
   }
 }
 
+bool AMDGPURewriteAGPRCopyMFMAImpl::insertImplicitDefsForLiveSegments(
+    LiveInterval &StackLI, int Slot, ArrayRef<MachineInstr *> SpillStores,
+    Register NewVReg) const {
+  const unsigned ScanLimit = ImplicitDefScanLimit;
+
+  SmallPtrSet<MachineInstr *, 4> StoreSet(SpillStores.begin(),
+                                          SpillStores.end());
+
+  SmallVector<MachineInstr *, 2> InsertedDefs;
+
+  for (const LiveRange::Segment &Seg : StackLI) {
+    MachineInstr *MI = LIS.getInstructionFromIndex(Seg.start);
+    // Fall back to LIS query in case the segment start does not correspond
+    // to an instruction.
+    MachineBasicBlock *MBB =
+        MI ? MI->getParent() : LIS.getMBBFromIndex(Seg.start);
+
+    // Bail-out: we could not resolve any MBB for this segment start.
+    // Roll back any defs already inserted for earlier segments -- the
+    // caller must skip unspilling this slot entirely.
+    if (!MBB) {
+      for (MachineInstr *Def : InsertedDefs) {
+        LIS.RemoveMachineInstrFromMaps(*Def);
+        Def->eraseFromParent();
+      }
+      return false;
+    }
+
+    // Scan forward from the segment start looking for a spill store to this
+    // slot. LiveStacks liveness for a store can start a few instructions
+    // before the store itself, so we allow a small window controlled by
+    // ImplicitDefScanLimit. Redundant IMPLICIT_DEFs (e.g. because of a small
+    // scan limit) are harmless, since they will be cleaned up by downstream
+    // transformations.
+    MachineBasicBlock::iterator It = MI ? MI->getIterator() : MBB->begin();
+    bool FoundStore = false;
+    for (unsigned I = 0; I < ScanLimit && It != MBB->end(); ++I, ++It) {
+      if (StoreSet.count(&*It)) {
+        FoundStore = true;
+        break;
+      }
+    }
----------------
ruiling wrote:

> If the execution is H -> S -> Latch -> H -> N -> Latch -> exit, the ultimate live-out value from the loop needs to match the value in S.

Does inserting implicit_def in `S` before the `spill-store` change this? I think no.

> This needs to be rewritten as a phi with an undef input on loop entry

For this case, the implicit_def in loop preheader is definitely necessary. But the second implicit_def in S is just the side-effect of a simple algorithm, there should be ways to avoid this. But it does not affect correctness. We are in non-SSA form. No need to reconstruct phi.

https://github.com/llvm/llvm-project/pull/198472