[RISCV] Let LiveIntervals::shrinkToUses compute dead AVLs

[lukel97]

We currently manually remove dead AVL defs if they are ADDIs used to materialize immediates > 31.
shrinkToUses already computes dead instructions though, so this patch uses it to generalize to any type of dead def.

As previously noted, shrinkToUses only checks if the instruction is dead so we need to also check if it's safe to use. Here we reuse the checks in LiveRangeEdit::eliminateDeadDef.

This fixes #95865 by removing the dead AVL from the LiveIntervals instruction map

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Luke Lau (lukel97)

Changes

We can simplify removing dead AVL immediates > 31 by using the dead argument to shrinkToUses, since it will already compute dead values.

---

Full diff: https://github.com/llvm/llvm-project/pull/90629.diff

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp (+3-11)

diff --git a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
index 216dc78085204e..f4ff17f7b65585 100644
--- a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
@@ -1648,17 +1648,9 @@ bool RISCVCoalesceVSETVLI::coalesceVSETVLIs(MachineBasicBlock &MBB) {
           if (NextMI->getOperand(1).isReg())
             NextMI->getOperand(1).setReg(RISCV::NoRegister);
 
-          if (OldVLReg && OldVLReg.isVirtual()) {
-            // NextMI no longer uses OldVLReg so shrink its LiveInterval.
-            LIS->shrinkToUses(&LIS->getInterval(OldVLReg));
-
-            MachineInstr *VLOpDef = MRI->getUniqueVRegDef(OldVLReg);
-            if (VLOpDef && TII->isAddImmediate(*VLOpDef, OldVLReg) &&
-                MRI->use_nodbg_empty(OldVLReg)) {
-              VLOpDef->eraseFromParent();
-              LIS->removeInterval(OldVLReg);
-            }
-          }
+          // NextMI no longer uses OldVLReg so shrink its LiveInterval.
+          if (OldVLReg && OldVLReg.isVirtual())
+            LIS->shrinkToUses(&LIS->getInterval(OldVLReg), &ToDelete);
           MI.setDesc(NextMI->getDesc());
         }
         MI.getOperand(2).setImm(NextMI->getOperand(2).getImm());

[preames]

I don't think this change is correct. It looks like ShrinkToUses only considers whether definitions are dead, and expects it's callers to validate that the instructions are safe to actually delete. This code doesn't do the required checks, and thus this seemingly obvious change actually introduces a correctness issue.

[lukel97]

Yeah, looks like RegisterCoalescer is the main user of this which eventually passes the dead instructions to LiveRangeEdit::eliminateDeadDefs, which in turn checks for bundles, inline asm and isSafeToMove

[lukel97]

This PR should now perform the same checks as LiveRangeEdit::eliminateDeadDef (isn't bundled, isn't asm, isSafeToMove)

[lukel97]

The issue reported in #95865 was caused by us forgetting to remove the dead AVL from the LiveIntervals instruction map, but it turns out this PR fixes this by reusing the same code path for the dead AVL and vsetvli. I've rebased this and included the reproducer as a test case cc @patrick-rivos