[RISCV] Update V0Defs after moving Src in peepholes

[lukel97]

If we move a pseudo in tryReduceVL or foldVMV_V_V via ensureDominates, its V0 definition may have changed so we need to update V0Defs.

This shouldn't have any functional change today since any pseudo which uses V0 won't be able to move past a new definition.

However this will matter if we add a peephole to convert unmasked pseudos to masked pseudos and add a use of V0.

[llvmbot]

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

Author: Luke Lau (lukel97)

Changes

If we move a pseudo in tryReduceVL or foldVMV_V_V via ensureDominates,
its V0 definition may have changed so we need to update V0Defs.

This shouldn't have any functional change today since any pseudo which
uses V0 won't be able to move past a new definition.

However this will matter if we add a peephole to convert unmasked
pseudos to masked pseudos and add a use of V0.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp (+5-5)

diff --git a/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp b/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp
index 026e0a365b8dcb..e6ea8c41269ca3 100644
--- a/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp
+++ b/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp
@@ -61,7 +61,7 @@ class RISCVVectorPeephole : public MachineFunctionPass {
   }
 
 private:
-  bool tryToReduceVL(MachineInstr &MI) const;
+  bool tryToReduceVL(MachineInstr &MI);
   bool convertToVLMAX(MachineInstr &MI) const;
   bool convertToWholeRegister(MachineInstr &MI) const;
   bool convertToUnmasked(MachineInstr &MI) const;
@@ -71,7 +71,7 @@ class RISCVVectorPeephole : public MachineFunctionPass {
 
   bool isAllOnesMask(const MachineInstr *MaskDef) const;
   std::optional<unsigned> getConstant(const MachineOperand &VL) const;
-  bool ensureDominates(const MachineOperand &Use, MachineInstr &Src) const;
+  bool ensureDominates(const MachineOperand &Use, MachineInstr &Src);
 
   /// Maps uses of V0 to the corresponding def of V0.
   DenseMap<const MachineInstr *, const MachineInstr *> V0Defs;
@@ -107,7 +107,7 @@ static unsigned getSEWLMULRatio(const MachineInstr &MI) {
 // Attempt to reduce the VL of an instruction whose sole use is feeding a
 // instruction with a narrower VL.  This currently works backwards from the
 // user instruction (which might have a smaller VL).
-bool RISCVVectorPeephole::tryToReduceVL(MachineInstr &MI) const {
+bool RISCVVectorPeephole::tryToReduceVL(MachineInstr &MI) {
   // Note that the goal here is a bit multifaceted.
   // 1) For store's reducing the VL of the value being stored may help to
   //    reduce VL toggles.  This is somewhat of an artifact of the fact we
@@ -457,7 +457,7 @@ static bool dominates(MachineBasicBlock::const_iterator A,
 /// does. Returns false if doesn't dominate and we can't move. \p MO must be in
 /// the same basic block as \Src.
 bool RISCVVectorPeephole::ensureDominates(const MachineOperand &MO,
-                                          MachineInstr &Src) const {
+                                          MachineInstr &Src) {
   assert(MO.getParent()->getParent() == Src.getParent());
   if (!MO.isReg() || MO.getReg() == RISCV::NoRegister)
     return true;
@@ -466,7 +466,7 @@ bool RISCVVectorPeephole::ensureDominates(const MachineOperand &MO,
   if (Def->getParent() == Src.getParent() && !dominates(Def, Src)) {
     if (!isSafeToMove(Src, *Def->getNextNode()))
       return false;
-    // FIXME: Update V0Defs
+    V0Defs[&Src] = V0Defs[Def];
     Src.moveBefore(Def->getNextNode());
   }
 

[lukel97]

I just want to point out that I agree with the discussion in https://github.com/llvm/llvm-project/pull/106753/files#r1739182733, it's probably worthwhile looking into a smaller mapping in a future PR, e.g. only store instructions that use V0.

[preames]

LGTM

For anyone curious, I was initially concerned about the case where Def might change V0 itself, but realized that's prevented by the safe to move check.

[lukel97]

LGTM

For anyone curious, I was initially concerned about the case where Def might change V0 itself, but realized that's prevented by the safe to move check.

Argh, now that you mention it this should actually be setting the definition to the one at Def->getNextNode().

[preames]

Argh, now that you mention it this should actually be setting the definition to the one at Def->getNextNode().

Yes, but per the reasoning in my comment those must be the same. :)