[RISCV] Teach vsetvli insertion to not insert redundant vsetvli right after VLEFF/VLSEGFF.

[fakepaper56]

Summary

VSETVLIInfos right after VLEFF/VLSEGFF are currently unknown since they modify
VL. Unknown VSETVLIInfos make next vector operations needed to be inserted
VSET(I)VLI. Actually the next vector operation of VLEFF/VLSEGFF may not need to
be inserted VSET(I)VLI if it uses same VTYPE and the resulted vl of
VLEFF/VLSEGFF.

Take the below C code as an example,

vint8m4_t vec_src1 = vle8ff_v_i8m4(str1, &new_vl, vl);
vbool2_t mask1 = vmseq_vx_i8m4_b2(vec_src1, 0, new_vl);
vsetvli insertion adds a redundant vsetvli for that,

Assembly result:

vsetvli a2,a2,e8,m4,ta,mu
vle8ff.v v28,(a0)
csrr a3,vl ; redundant
vsetvli zero,a3,e8,m4,ta,mu ; redundant
vmseq.vi v25,v28,0

After D126794, VLEFF/VLSEGFF has a define having value of VL. The patch consider
there is a ghost vsetvli right after VLEFF/VLSEGFF. The ghost VSET(I)LIs use the
vl output of the VLEFF/VLSEGFF as its AVL and same VTYPE of the VLEFF/VLSEGFF.
The ghost vsetvli must be redundant, and we could use it to get the VSETVLIInfo
right after VLEFF/VLSEGFF.

[fakepaper56]

Refine the commit message.

[craig.topper]

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

934

Please rebase now that isFaultFirstLoad isn't in RISCVInstrInfo.

1085

Can we split this if and combine it with the one below? By doing something like

if (!DefMI)
  return true;

if (isVectorConfigInstr(*DefMI)) {
  DefInfo = getInfoForVSETVLI(*DefMI);
} else if (TII->isFaultFirstLoad(*DefMI)) {
  DefInfo = computeInfoForInstr(*DefMI, DefMI->getDesc().TSFlags, MRI);
  DefInfo.setAVLReg(DefMI->getOperand(1).getReg());
} else {
  return true;
}

Along with appropriate modifications to the comments.

1149–1150

This code is identical to the code on line 975, except for which varaible we update. Could this use a shared function?

[fakepaper56]

Address Craig's comments.

1. Add function updateToEndStatus to share code for updating VSETVLIInfo. Does somebody have better naming to represent the status between one MI and the next one?
2. Refine code of needVSETVLIPHI.

[fakepaper56]

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

934

Done.

1085

Done.

[fakepaper56]

Refine the comments.

[reames]

Three high level concerns with this patch.

First, you need to rework the patch description. As written, it sounds like you're trying to fix a functional issue. To the best of my knowledge, that does not appear to actually be the case. Instead, this patch is improving precision for the case where VL is modified by a FF instruction.

Second, this change mixes at least three separate optimizations. I've tagged everything which isn't the transfer rule, please remove them from the initial patch.

Third, we have three places which check for modifiesRegister(VL). You only update two of them. Make sure you include the third.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

327

This should not be a member function on VSETVLIInfo. Its part of the data flow transfer rule, not the abstract state.

934

This is a separate optimization, move it to its own review.

1087

This is a separate optimization, move it to its own review.

[fakepaper56]

Address reames's comments.

[fakepaper56]

Refine commit summary.

[fakepaper56]

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

327

Done.

934

Done. And sorry the code is redundant.

1087

Done.

[reames]

LGTM w/required change.

llvm/test/CodeGen/RISCV/rvv/vsetvli-modify-vl.ll

1 ↗

(On Diff #436686)

It's not clear to me what this test is covering that the one above doesn't. It appears to be the same except for an extract vmseq? You might consider dropping it entirely.

Either way, it shouldn't be it's own file. Add to the end of vsetvli-insert.ll with a refined name please.

[fakepaper56]

Rebase to origin/main and move the new test case into vsetvli-insert.ll.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

bool isCompatible(const MachineInstr &MI, const VSETVLIInfo &Require) const {
     if (StoreOp && VLMul == Require.VLMul && SEW == Require.SEW)
       return true;
 
     // Anything else is not compatible.
     return false;
   }
 
   bool isCompatibleWithLoadStoreEEW(unsigned EEW,
                                     const VSETVLIInfo &Require) const {
     assert(isValid() && Require.isValid() &&
            "Can't compare invalid VSETVLIInfos");
     assert(!Require.SEWLMULRatioOnly &&
            "Expected a valid VTYPE for instruction!");
     assert(EEW == Require.SEW && "Mismatched EEW/SEW for store");
 
     if (isUnknown() || hasSEWLMULRatioOnly())
       return false;
if (!CurInfo.isUnknown() && Require.hasAVLReg() &&
       CurInfo.hasCompatibleVTYPE(MI, Require)) {
     if (MachineInstr *DefMI = MRI->getVRegDef(Require.getAVLReg())) {
       if (isVectorConfigInstr(*DefMI)) {
         VSETVLIInfo DefInfo = getInfoForVSETVLI(*DefMI);
         if (DefInfo.hasSameAVL(CurInfo) && DefInfo.hasSameVLMAX(CurInfo))
           return false;
       }
     }
   }
 
   // If this is a unit-stride or strided load/store, we may be able to use the
   // EMUL=(EEW/SEW)*LMUL relationship to avoid changing VTYPE.
   return CurInfo.isUnknown() || !canSkipVSETVLIForLoadStore(MI, Require, CurInfo);
 }
 
 // Given an incoming state reaching MI, modifies that state so that it is minimally
 // compatible with MI.  The resulting state is guaranteed to be semantically legal

 // this might be different that the state MI requested), modify the state to
 // reflect the changes MI might make.
 void RISCVInsertVSETVLI::transferAfter(VSETVLIInfo &Info, const MachineInstr &MI) {
   if (isVectorConfigInstr(MI)) {
     Info = getInfoForVSETVLI(MI);
     return;
   }
 
+  if (RISCV::isFaultFirstLoad(MI)) {
+    // Update AVL to vl-output of the fault first load.
+    Info.setAVLReg(MI.getOperand(1).getReg());
+    return;
+  }
+
   // If this is something that updates VL/VTYPE that we don't know about, set
   // the state to unknown.
   if (MI.isCall() || MI.isInlineAsm() || MI.modifiesRegister(RISCV::VL) ||
       MI.modifiesRegister(RISCV::VTYPE))
     Info = VSETVLIInfo::getUnknown();
 }
 
 bool RISCVInsertVSETVLI::computeVLVTYPEChanges(const MachineBasicBlock &MBB) {
for (unsigned PHIOp = 1, NumOps = PHI->getNumOperands(); PHIOp != NumOps;
     // If the exit from the predecessor has the VTYPE we are looking for
     // we might be able to avoid a VSETVLI.
     if (PBBInfo.Exit.isUnknown() || !PBBInfo.Exit.hasSameVTYPE(Require))
       return true;
 
     // We need the PHI input to the be the output of a VSET(I)VLI.
     MachineInstr *DefMI = MRI->getVRegDef(InReg);
     if (!DefMI || !isVectorConfigInstr(*DefMI))
       return true;
 
     // We found a VSET(I)VLI make sure it matches the output of the
     // predecessor block.
     VSETVLIInfo DefInfo = getInfoForVSETVLI(*DefMI);
     if (!DefInfo.hasSameAVL(PBBInfo.Exit) ||
         !DefInfo.hasSameVTYPE(PBBInfo.Exit))
       return true;
   }
 
   // If all the incoming values to the PHI checked out, we don't need
if (RISCVII::hasSEWOp(TSFlags)) {
         }
         MI.addOperand(MachineOperand::CreateReg(RISCV::VL, /*isDef*/ false,
                                                 /*isImp*/ true));
       }
       MI.addOperand(MachineOperand::CreateReg(RISCV::VTYPE, /*isDef*/ false,
                                               /*isImp*/ true));
     }
 
     if (MI.isCall() || MI.isInlineAsm() || MI.modifiesRegister(RISCV::VL) ||
         MI.modifiesRegister(RISCV::VTYPE))
       PrefixTransparent = false;
 
     transferAfter(CurInfo, MI);
   }
 
   // If we reach the end of the block and our current info doesn't match the
   // expected info, insert a vsetvli to correct.
   if (!UseStrictAsserts) {
if (RISCVII::hasSEWOp(TSFlags)) {
             }
           }
         }
       }
       CurInfo = computeInfoForInstr(MI, TSFlags, MRI);
       continue;
     }
 
-    // If this is something that updates VL/VTYPE that we don't know about,
-    // set the state to unknown.
-    if (MI.isCall() || MI.isInlineAsm() || MI.modifiesRegister(RISCV::VL) ||
-        MI.modifiesRegister(RISCV::VTYPE))
-      CurInfo = VSETVLIInfo::getUnknown();
+    transferAfter(CurInfo, MI);
   }
 }
 
 /// Return true if the VL value configured must be equal to the requested one.
 static bool hasFixedResult(const VSETVLIInfo &Info, const RISCVSubtarget &ST) {
   if (!Info.hasAVLImm())
     // VLMAX is always the same value.
     // TODO: Could extend to other registers by looking at the associated vreg

llvm/test/CodeGen/RISCV/rvv/vsetvli-insert.ll

 ; CHECK-NEXT:    ret
 entry:
   %vl = tail call i64 @llvm.riscv.vsetvli(i64 %reg, i64 1, i64 0)
   call void @llvm.riscv.vse.nxv2i32.i64(<vscale x 2 x i32> %v, <vscale x 2 x i32>* %p, i64 %vl)
   ret i64 %vl
 }
 
 ; Fault first loads can modify VL.
-; TODO: The first and third VSETVLIs are redundant here.
+; TODO: The VSETVLI of vadd could be removed here.
 define <vscale x 1 x i64> @vleNff(i64* %str, i64 %n, i64 %x) {
 ; CHECK-LABEL: vleNff:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    vsetvli zero, a1, e64, m1, ta, mu
 ; CHECK-NEXT:    vle64ff.v v8, (a0)
-; CHECK-NEXT:    csrr a0, vl
-; CHECK-NEXT:    vsetvli zero, a0, e64, m1, tu, mu
+; CHECK-NEXT:    vsetvli zero, zero, e64, m1, tu, mu
 ; CHECK-NEXT:    vadd.vx v8, v8, a2
 ; CHECK-NEXT:    ret
 entry:
   %0 = tail call i64 @llvm.riscv.vsetvli.i64(i64 %n, i64 0, i64 2)
   %1 = bitcast i64* %str to <vscale x 1 x i64>*
   %2 = tail call { <vscale x 1 x i64>, i64 } @llvm.riscv.vleff.nxv1i64.i64(<vscale x 1 x i64> undef, <vscale x 1 x i64>* %1, i64 %0)
   %3 = extractvalue { <vscale x 1 x i64>, i64 } %2, 0
   %4 = extractvalue { <vscale x 1 x i64>, i64 } %2, 1
   %5 = tail call <vscale x 1 x i64> @llvm.riscv.vadd.nxv1i64.i64.i64(<vscale x 1 x i64> %3, <vscale x 1 x i64> %3, i64 %x, i64 %4)
   ret <vscale x 1 x i64> %5
 }
 
+; Similiar test case, but use same policy for vleff and vadd.
+; Note: The test may be redundant if we could fix the TODO of @vleNff.
+define <vscale x 1 x i64> @vleNff2(i64* %str, i64 %n, i64 %x) {
+; CHECK-LABEL: vleNff2:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    vsetvli zero, a1, e64, m1, ta, mu
+; CHECK-NEXT:    vle64ff.v v8, (a0)
+; CHECK-NEXT:    vadd.vx v8, v8, a2
+; CHECK-NEXT:    ret
+entry:
+  %0 = tail call i64 @llvm.riscv.vsetvli.i64(i64 %n, i64 0, i64 2)
+  %1 = bitcast i64* %str to <vscale x 1 x i64>*
+  %2 = tail call { <vscale x 1 x i64>, i64 } @llvm.riscv.vleff.nxv1i64.i64(<vscale x 1 x i64> undef, <vscale x 1 x i64>* %1, i64 %0)
+  %3 = extractvalue { <vscale x 1 x i64>, i64 } %2, 0
+  %4 = extractvalue { <vscale x 1 x i64>, i64 } %2, 1
+  %5 = tail call <vscale x 1 x i64> @llvm.riscv.vadd.nxv1i64.i64.i64(<vscale x 1 x i64> undef, <vscale x 1 x i64> %3, i64 %x, i64 %4)
+  ret <vscale x 1 x i64> %5
+}
+
 declare { <vscale x 1 x i64>, i64 } @llvm.riscv.vleff.nxv1i64.i64(
   <vscale x 1 x i64>, <vscale x 1 x i64>* nocapture, i64)
 
 declare <vscale x 1 x i1> @llvm.riscv.vmseq.nxv1i64.i64.i64(
   <vscale x 1 x i64>, i64, i64)
 
 ; Ensure AVL register is alive when forwarding an AVL immediate that does not fit in 5 bits
 define <vscale x 2 x i32> @avl_forward5(<vscale x 2 x i32>* %addr) {