[NFC][RISCV] Keep AVLReg define instr inside VSETVLInfo #89180

BeMg · 2024-04-18T07:46:56Z

Currently, the vsetvli pass tracks the define instruction through MRI->getVRegDef due to the SSA form.

This patch keeps the AVLReg DefMI within VSETVLInfo during construction. And replace MRI->getVRegDef(AVLReg) with getAVLRegDefMI().

This information is useful when vsetvli pass live in post-ra situation.

The testcases don't change because the VReg always has a unique def in SSA.

llvmbot · 2024-04-18T07:47:26Z

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

Author: Piyou Chen (BeMg)

Changes

Currently, the vsetvli pass tracks the define instruction through MRI->getVRegDef due to the SSA form.

This patch keeps the AVLReg DefMI within VSETVLInfo during construction. And replace MRI->getVRegDef(AVLReg) with getAVLRegDefMI().

This information is useful when vsetvli pass live in post-ra situation.

The testcases don't change because the VReg always has a unique def in SSA.

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

1 Files Affected:

(modified) llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp (+35-21)

diff --git a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
index 331253e39c0acb..222123aa706e4f 100644
--- a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
@@ -153,7 +153,7 @@ static std::optional<unsigned> getEEWForLoadStore(const MachineInstr &MI) {
   }
 }
 
-static bool isNonZeroLoadImmediate(MachineInstr &MI) {
+static bool isNonZeroLoadImmediate(const MachineInstr &MI) {
   return MI.getOpcode() == RISCV::ADDI &&
     MI.getOperand(1).isReg() && MI.getOperand(2).isImm() &&
     MI.getOperand(1).getReg() == RISCV::X0 &&
@@ -438,6 +438,8 @@ class VSETVLIInfo {
     unsigned AVLImm;
   };
 
+  const MachineInstr *AVLDefMI;
+
   enum : uint8_t {
     Uninitialized,
     AVLIsReg,
@@ -454,7 +456,7 @@ class VSETVLIInfo {
 
 public:
   VSETVLIInfo()
-      : AVLImm(0), TailAgnostic(false), MaskAgnostic(false),
+      : AVLImm(0), AVLDefMI(nullptr), TailAgnostic(false), MaskAgnostic(false),
         SEWLMULRatioOnly(false) {}
 
   static VSETVLIInfo getUnknown() {
@@ -478,6 +480,8 @@ class VSETVLIInfo {
     State = AVLIsImm;
   }
 
+  void setAVLDefMI(const MachineInstr *DefMI) { AVLDefMI = DefMI; }
+
   bool hasAVLImm() const { return State == AVLIsImm; }
   bool hasAVLReg() const { return State == AVLIsReg; }
   Register getAVLReg() const {
@@ -489,13 +493,16 @@ class VSETVLIInfo {
     return AVLImm;
   }
 
+  const MachineInstr *getAVLDefMI() const { return AVLDefMI; }
+
   void setAVL(VSETVLIInfo Info) {
     assert(Info.isValid());
     if (Info.isUnknown())
       setUnknown();
-    else if (Info.hasAVLReg())
+    else if (Info.hasAVLReg()) {
       setAVLReg(Info.getAVLReg());
-    else {
+      setAVLDefMI(Info.getAVLDefMI());
+    } else {
       assert(Info.hasAVLImm());
       setAVLImm(Info.getAVLImm());
     }
@@ -512,8 +519,7 @@ class VSETVLIInfo {
     if (hasAVLReg()) {
       if (getAVLReg() == RISCV::X0)
         return true;
-      if (MachineInstr *MI = MRI.getVRegDef(getAVLReg());
-          MI && isNonZeroLoadImmediate(*MI))
+      if (getAVLDefMI() && isNonZeroLoadImmediate(*getAVLDefMI()))
         return true;
       return false;
     }
@@ -792,7 +798,8 @@ INITIALIZE_PASS(RISCVInsertVSETVLI, DEBUG_TYPE, RISCV_INSERT_VSETVLI_NAME,
 
 // Return a VSETVLIInfo representing the changes made by this VSETVLI or
 // VSETIVLI instruction.
-static VSETVLIInfo getInfoForVSETVLI(const MachineInstr &MI) {
+static VSETVLIInfo getInfoForVSETVLI(const MachineInstr &MI,
+                                     const MachineRegisterInfo &MRI) {
   VSETVLIInfo NewInfo;
   if (MI.getOpcode() == RISCV::PseudoVSETIVLI) {
     NewInfo.setAVLImm(MI.getOperand(1).getImm());
@@ -803,6 +810,8 @@ static VSETVLIInfo getInfoForVSETVLI(const MachineInstr &MI) {
     assert((AVLReg != RISCV::X0 || MI.getOperand(0).getReg() != RISCV::X0) &&
            "Can't handle X0, X0 vsetvli yet");
     NewInfo.setAVLReg(AVLReg);
+    if (AVLReg.isVirtual())
+      NewInfo.setAVLDefMI(MRI.getVRegDef(AVLReg));
   }
   NewInfo.setVTYPE(MI.getOperand(2).getImm());
 
@@ -875,6 +884,8 @@ static VSETVLIInfo computeInfoForInstr(const MachineInstr &MI, uint64_t TSFlags,
         InstrInfo.setAVLImm(Imm);
     } else {
       InstrInfo.setAVLReg(VLOp.getReg());
+      if (VLOp.getReg().isVirtual())
+        InstrInfo.setAVLDefMI(MRI->getVRegDef(VLOp.getReg()));
     }
   } else {
     assert(isScalarExtractInstr(MI));
@@ -892,9 +903,10 @@ static VSETVLIInfo computeInfoForInstr(const MachineInstr &MI, uint64_t TSFlags,
   // register AVLs to avoid extending live ranges without being sure we can
   // kill the original source reg entirely.
   if (InstrInfo.hasAVLReg() && InstrInfo.getAVLReg().isVirtual()) {
-    MachineInstr *DefMI = MRI->getVRegDef(InstrInfo.getAVLReg());
-    if (DefMI && isVectorConfigInstr(*DefMI)) {
-      VSETVLIInfo DefInstrInfo = getInfoForVSETVLI(*DefMI);
+    if (InstrInfo.getAVLDefMI() &&
+        isVectorConfigInstr(*InstrInfo.getAVLDefMI())) {
+      VSETVLIInfo DefInstrInfo =
+          getInfoForVSETVLI(*InstrInfo.getAVLDefMI(), *MRI);
       if (DefInstrInfo.hasSameVLMAX(InstrInfo) &&
           (DefInstrInfo.hasAVLImm() || DefInstrInfo.getAVLReg() == RISCV::X0)) {
         InstrInfo.setAVL(DefInstrInfo);
@@ -934,9 +946,9 @@ void RISCVInsertVSETVLI::insertVSETVLI(MachineBasicBlock &MBB,
     // same, we can use the X0, X0 form.
     if (Info.hasSameVLMAX(PrevInfo) && Info.hasAVLReg() &&
         Info.getAVLReg().isVirtual()) {
-      if (MachineInstr *DefMI = MRI->getVRegDef(Info.getAVLReg())) {
-        if (isVectorConfigInstr(*DefMI)) {
-          VSETVLIInfo DefInfo = getInfoForVSETVLI(*DefMI);
+      if (Info.getAVLDefMI()) {
+        if (isVectorConfigInstr(*Info.getAVLDefMI())) {
+          VSETVLIInfo DefInfo = getInfoForVSETVLI(*Info.getAVLDefMI(), *MRI);
           if (DefInfo.hasSameAVL(PrevInfo) && DefInfo.hasSameVLMAX(PrevInfo)) {
             BuildMI(MBB, InsertPt, DL, TII->get(RISCV::PseudoVSETVLIX0))
                 .addReg(RISCV::X0, RegState::Define | RegState::Dead)
@@ -1056,9 +1068,9 @@ bool RISCVInsertVSETVLI::needVSETVLI(const MachineInstr &MI,
   // VSETVLI here.
   if (Require.hasAVLReg() && Require.getAVLReg().isVirtual() &&
       CurInfo.hasCompatibleVTYPE(Used, Require)) {
-    if (MachineInstr *DefMI = MRI->getVRegDef(Require.getAVLReg())) {
-      if (isVectorConfigInstr(*DefMI)) {
-        VSETVLIInfo DefInfo = getInfoForVSETVLI(*DefMI);
+    if (Require.getAVLDefMI()) {
+      if (isVectorConfigInstr(*Require.getAVLDefMI())) {
+        VSETVLIInfo DefInfo = getInfoForVSETVLI(*Require.getAVLDefMI(), *MRI);
         if (DefInfo.hasSameAVL(CurInfo) && DefInfo.hasSameVLMAX(CurInfo))
           return false;
       }
@@ -1145,13 +1157,15 @@ void RISCVInsertVSETVLI::transferBefore(VSETVLIInfo &Info,
 void RISCVInsertVSETVLI::transferAfter(VSETVLIInfo &Info,
                                        const MachineInstr &MI) const {
   if (isVectorConfigInstr(MI)) {
-    Info = getInfoForVSETVLI(MI);
+    Info = getInfoForVSETVLI(MI, *MRI);
     return;
   }
 
   if (RISCV::isFaultFirstLoad(MI)) {
     // Update AVL to vl-output of the fault first load.
     Info.setAVLReg(MI.getOperand(1).getReg());
+    if (MI.getOperand(1).getReg().isVirtual())
+      Info.setAVLDefMI(MRI->getVRegDef(MI.getOperand(1).getReg()));
     return;
   }
 
@@ -1270,7 +1284,7 @@ bool RISCVInsertVSETVLI::needVSETVLIPHI(const VSETVLIInfo &Require,
 
     // We found a VSET(I)VLI make sure it matches the output of the
     // predecessor block.
-    VSETVLIInfo DefInfo = getInfoForVSETVLI(*DefMI);
+    VSETVLIInfo DefInfo = getInfoForVSETVLI(*DefMI, *MRI);
     if (!DefInfo.hasSameAVL(PBBInfo.Exit) ||
         !DefInfo.hasSameVTYPE(PBBInfo.Exit))
       return true;
@@ -1418,7 +1432,7 @@ void RISCVInsertVSETVLI::doPRE(MachineBasicBlock &MBB) {
   // we need to prove the value is available at the point we're going
   // to insert the vsetvli at.
   if (AvailableInfo.hasAVLReg() && RISCV::X0 != AvailableInfo.getAVLReg()) {
-    MachineInstr *AVLDefMI = MRI->getVRegDef(AvailableInfo.getAVLReg());
+    const MachineInstr *AVLDefMI = AvailableInfo.getAVLDefMI();
     if (!AVLDefMI)
       return;
     // This is an inline dominance check which covers the case of
@@ -1504,8 +1518,8 @@ static bool canMutatePriorConfig(const MachineInstr &PrevMI,
     if (Used.VLZeroness) {
       if (isVLPreservingConfig(PrevMI))
         return false;
-      if (!getInfoForVSETVLI(PrevMI).hasEquallyZeroAVL(getInfoForVSETVLI(MI),
-                                                       MRI))
+      if (!getInfoForVSETVLI(PrevMI, MRI)
+               .hasEquallyZeroAVL(getInfoForVSETVLI(MI, MRI), MRI))
         return false;
     }

lukel97

Should the PR title be marked with NFC

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

We currently use AVLIsReg to represent VLMAX as well as a dummy value for whenever the VL is ignored by vmv.x.s. This splits them out into separate states so that AVLIsReg is always a virtual register and should help with tracking the definition inside VSETVLIInfo directly in llvm#89180. This is almost an NFC but it sets the kill flag for x0 in more places.

BeMg · 2024-04-25T06:14:10Z

Base on #89964, replace AVLIsReg with AVLIsDefMI.

We currently use AVLIsReg to represent VLMAX as well as a dummy value for whenever the VL is ignored by vmv.x.s. This splits them out into separate states so that AVLIsReg is always a virtual register and should help with tracking the definition inside VSETVLIInfo directly in #89180. This is almost an NFC but it sets the kill flag for x0 in more places.

lukel97 · 2024-04-25T06:25:52Z

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

    return;
  }

  if (RISCV::isFaultFirstLoad(MI)) {
    // Update AVL to vl-output of the fault first load.
-    Info.setAVLReg(MI.getOperand(1).getReg());
+    Info.setAVLDefMI(MRI->getVRegDef(MI.getOperand(1).getReg()));


Just making a note, I don't think we have any pseudos that have more than one scalar def, but if we did then we would also need to track the operand number.

lukel97 · 2024-04-25T06:28:33Z

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

-      VSETVLIInfo DefInstrInfo = getInfoForVSETVLI(*DefMI);
+  if (InstrInfo.hasAVLDefMI()) {
+    const MachineInstr *DefMI = InstrInfo.getAVLDefMI();
+    if (DefMI && isVectorConfigInstr(*DefMI)) {


DefMI should never be null, what do you think about changing getAVLDefMI() to return a const reference? Looks like a lot of the uses require dereferencing it anyway.

lukel97 · 2024-04-25T06:31:24Z

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

-    assert(hasAVLReg());
-    return AVLReg;
+    assert(hasAVLDefMI());
+    return AVLDefMI->getOperand(0).getReg();


If the AVL comes from the def of a fault first load then I think it will come from the first operand. I think we might need to track the operand number then in that case?

How about keeping both AVLReg and AVLDefMI inside VSETVLInfo? Tracking the operand number looks too indirect because we just want AVLReg here.

RISCVInsertVSETVLI::insertVSETVLI is only place need AVLReg actually.

Register AVLReg = Info.getAVLReg(); MRI->constrainRegClass(AVLReg, &RISCV::GPRNoX0RegClass); BuildMI(MBB, InsertPt, DL, TII->get(RISCV::PseudoVSETVLI)) .addReg(RISCV::X0, RegState::Define | RegState::Dead) .addReg(AVLReg) .addImm(Info.encodeVTYPE());

Yeah that also works, as long as we can fit it inside the existing union, something like this maybe?

struct Def { const MachineInstr *DefMI; Register Reg; }; union { Def AVLDef; unsigned AVLImm; };

Would need to also update the == and AVL comparison methods to check the register too I think

It would be nice to use std::pair here but it has a non-trivial copy constructor unfortunately

BeMg · 2024-04-25T07:10:37Z

Rebase with main branch

BeMg · 2024-04-25T11:14:17Z

Update

Use the const reference for getAVLDefMI
Keep both AVLReg and AVLRegDefMI inside VSETVLInfo
Rename some function
Move AVLReg AVLRegDefMI assert check into getAVLDefMI, setAVLRegDef and getAVLReg

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

lukel97

LGTM. Hopefully then in #70549 we can replace the AVLDef struct with VNInfo or whatever is needed to track definitions post-SSA.

BeMg · 2024-04-26T05:52:08Z

Rebase with main

Currently, the vsetvli pass track the define instruction through MRI->getVRegDef due to the SSA form. This patch keeps the AVLReg DefMI within VSETVLInfo during construction. And replace MRI->getVRegDef(AVLReg) with getAVLRegDefMI(). This information is useful when vsetvli pass live in post-ra situation. The testcases don't change because the VReg always has a unique def in SSA.

BeMg requested review from preames, lukel97 and topperc April 18, 2024 07:46

llvmbot added the backend:RISC-V label Apr 18, 2024

BeMg mentioned this pull request Apr 18, 2024

[RISCV] Support postRA vsetvl insertion pass #70549

Merged

lukel97 reviewed Apr 22, 2024

View reviewed changes

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp Outdated Show resolved Hide resolved

BeMg changed the title ~~[RISCV] Keep AVLReg define instr inside VSETVLInfo~~ [NFC][RISCV] Keep AVLReg define instr inside VSETVLInfo Apr 22, 2024

lukel97 reviewed Apr 24, 2024

View reviewed changes

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp Outdated Show resolved Hide resolved

BeMg force-pushed the keep-AVLDefMI-inside-VSETVLInfo branch from cee023a to cae0c63 Compare April 24, 2024 14:33

lukel97 mentioned this pull request Apr 24, 2024

[RISCV] Split out VSETVLIInfo AVL states to be more explicit #89964

Merged

BeMg force-pushed the keep-AVLDefMI-inside-VSETVLInfo branch from cae0c63 to efc895c Compare April 25, 2024 06:12

lukel97 reviewed Apr 25, 2024

View reviewed changes

BeMg force-pushed the keep-AVLDefMI-inside-VSETVLInfo branch from efc895c to 25ecf90 Compare April 25, 2024 07:07

BeMg requested a review from lukel97 April 25, 2024 11:14

lukel97 reviewed Apr 25, 2024

View reviewed changes

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp Outdated Show resolved Hide resolved

lukel97 approved these changes Apr 26, 2024

View reviewed changes

BeMg force-pushed the keep-AVLDefMI-inside-VSETVLInfo branch from bfbb42a to 5c42b12 Compare April 26, 2024 05:51

BeMg force-pushed the keep-AVLDefMI-inside-VSETVLInfo branch from 5c42b12 to c5522ce Compare April 26, 2024 07:14

BeMg added 5 commits April 27, 2024 02:07

Get rid of MRI from hasNonZeroAVL and hasEquallyZeroAVL

816fa6b

Merge setDefMI into setReg

e19a2db

Replace the setAVLReg and AVLIsReg with DefMI

2182cf9

Store both AVLReg and AVLRegDefMI

a5074f5

BeMg added 2 commits April 27, 2024 02:07

Use const reference for getAVLDefMI and remove some useless assert

4ed7160

hasSameAVL also consider AVLReg

bedde94

BeMg force-pushed the keep-AVLDefMI-inside-VSETVLInfo branch from c5522ce to bedde94 Compare April 27, 2024 09:25

BeMg merged commit 5820ad9 into llvm:main Apr 28, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC][RISCV] Keep AVLReg define instr inside VSETVLInfo #89180

[NFC][RISCV] Keep AVLReg define instr inside VSETVLInfo #89180

BeMg commented Apr 18, 2024

llvmbot commented Apr 18, 2024

lukel97 left a comment

BeMg commented Apr 25, 2024

lukel97 Apr 25, 2024

lukel97 Apr 25, 2024

lukel97 Apr 25, 2024

BeMg Apr 25, 2024

lukel97 Apr 25, 2024

lukel97 Apr 25, 2024

BeMg commented Apr 25, 2024

BeMg commented Apr 25, 2024

lukel97 left a comment

BeMg commented Apr 26, 2024

[NFC][RISCV] Keep AVLReg define instr inside VSETVLInfo #89180

[NFC][RISCV] Keep AVLReg define instr inside VSETVLInfo #89180

Conversation

BeMg commented Apr 18, 2024

llvmbot commented Apr 18, 2024

lukel97 left a comment

Choose a reason for hiding this comment

BeMg commented Apr 25, 2024

lukel97 Apr 25, 2024

Choose a reason for hiding this comment

lukel97 Apr 25, 2024

Choose a reason for hiding this comment

lukel97 Apr 25, 2024

Choose a reason for hiding this comment

BeMg Apr 25, 2024

Choose a reason for hiding this comment

lukel97 Apr 25, 2024

Choose a reason for hiding this comment

lukel97 Apr 25, 2024

Choose a reason for hiding this comment

BeMg commented Apr 25, 2024

BeMg commented Apr 25, 2024

lukel97 left a comment

Choose a reason for hiding this comment

BeMg commented Apr 26, 2024