[RISCV] Don't set AVL if only zeroness is demanded

[lukel97]

This refactors the logic in transferBefore so that we're moving in the direction of "keep the existing Info, only change what is needed".

For the sake of review there are two commits in this PR: The former is needed to make the latter an NFC commit. Neither introduce any test diffs but the former is not technically NFC, hence why I did not precommit it.

• [RISCV] Preserve AVL when previous info is ratio only in transferBefore
• [RISCV] Don't change AVL if only zeroness is demanded. NFC

[llvmbot]

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

Author: Luke Lau (lukel97)

Changes

This refactors the logic in transferBefore so that we're moving in the direction of "keep the existing Info, only change what is needed".

For the sake of review there are two commits in this PR: The former is needed to make the latter an NFC commit. Neither introduce any test diffs but the former is not technically NFC, hence why I did not precommit it.

• [RISCV] Preserve AVL when previous info is ratio only in transferBefore
• [RISCV] Don't change AVL if only zeroness is demanded. NFC

---

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

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp (+19-16)

diff --git a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
index 323a92cfb8c83d3..5e239ff06371e9a 100644
--- a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
@@ -1061,7 +1061,7 @@ void RISCVInsertVSETVLI::transferBefore(VSETVLIInfo &Info,
     return;
 
   const VSETVLIInfo PrevInfo = Info;
-  if (Info.hasSEWLMULRatioOnly() || !Info.isValid() || Info.isUnknown())
+  if (!Info.isValid() || Info.isUnknown())
     Info = NewInfo;
 
   if (!RISCVII::hasVLOp(TSFlags)) {
@@ -1073,7 +1073,16 @@ void RISCVInsertVSETVLI::transferBefore(VSETVLIInfo &Info,
   const VSETVLIInfo IncomingInfo =
       adjustIncoming(PrevInfo, NewInfo, Demanded, MRI);
 
-  if (Demanded.usedVL())
+  // If MI only demands that VL has the same zeroness, we only need to set the
+  // AVL if the zeroness differs.  This removes a vsetvli entirely if the types
+  // match or allows use of cheaper avl preserving variant if VLMAX doesn't
+  // change. If VLMAX might change, we couldn't use the 'vsetvli x0, x0, vtype"
+  // variant, so we avoid the transform to prevent extending live range of an
+  // avl register operand.
+  // TODO: We can probably relax this for immediates.
+  bool EquallyZero = IncomingInfo.hasEquallyZeroAVL(PrevInfo, *MRI) &&
+                     IncomingInfo.hasSameVLMAX(PrevInfo);
+  if (Demanded.VLAny || (Demanded.VLZeroness && !EquallyZero))
     Info.setAVL(IncomingInfo);
 
   Info.setVTYPE(
@@ -1086,6 +1095,14 @@ void RISCVInsertVSETVLI::transferBefore(VSETVLIInfo &Info,
           IncomingInfo.getTailAgnostic(),
       (Demanded.MaskPolicy ? IncomingInfo : Info).getMaskAgnostic() ||
           IncomingInfo.getMaskAgnostic());
+
+  // If we only knew the sew/lmul ratio previously, replace the VTYPE but keep
+  // the AVL.
+  if (Info.hasSEWLMULRatioOnly()) {
+    VSETVLIInfo RatiolessInfo = IncomingInfo;
+    RatiolessInfo.setAVL(Info);
+    Info = RatiolessInfo;
+  }
 }
 
 static VSETVLIInfo adjustIncoming(VSETVLIInfo PrevInfo, VSETVLIInfo NewInfo,
@@ -1104,20 +1121,6 @@ static VSETVLIInfo adjustIncoming(VSETVLIInfo PrevInfo, VSETVLIInfo NewInfo,
     Demanded.LMUL = true;
   }
 
-  // If we only demand VL zeroness (i.e. vmv.s.x and vmv.x.s), then there are
-  // only two behaviors, VL = 0 and VL > 0. We can discard the user requested
-  // AVL and just use the last one if we can prove it equally zero. This
-  // removes a vsetvli entirely if the types match or allows use of cheaper avl
-  // preserving variant if VLMAX doesn't change. If VLMAX might change, we
-  // couldn't use the 'vsetvli x0, x0, vtype" variant, so we avoid the transform
-  // to prevent extending live range of an avl register operand.
-  // TODO: We can probably relax this for immediates.
-  if (Demanded.VLZeroness && !Demanded.VLAny && PrevInfo.isValid() &&
-      PrevInfo.hasEquallyZeroAVL(Info, *MRI) && Info.hasSameVLMAX(PrevInfo)) {
-    Info.setAVL(PrevInfo);
-    Demanded.demandVL();
-  }
-
   return Info;
 }
 

[preames]

I think this code actually has a bug. If PrevInfo is SEWLMULRatioOnly, inspecting it's AVL is highly suspect. That AVL is purely a garbage value, and I don't know we're even guaranteed for it to be the prior active AVL (consider a meet on a branch with different AVLs).

[preames]

I am wrong here. I had misread the definition of SEWLMULRatioOnly. This flag is only set in the merge if the AVL is the same, and thus well defined.

[preames]

LGTM w/suggestion.

[preames]

Is there any value having this here as opposed to as an early exit before we call adjustIncoming?

[lukel97]

We have to handle the case where the AVL might be preserved so we can't bail before the AVL gets set on line 1084. We could maybe do an early exit after that though, before we set VTYPE.

As a side note, this is a bit awkward because calling setVTYPE isn't enough to clear the hasSEWLMULRatioOnly bit. Hence why we need to copy over incoming info.

[topperc]

I recently pulled this patch in our downstream and I'm seeing an assertion failure

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:561: unsigned int (anonymous namespace)::VSETVLIInfo::encodeVTYPE() const: Assertion `isValid() && !isUnknown() && !SEWLMULRatioOnly && "Can't encode VTYPE for uninitialized or unknown"' failed.

[topperc]

Here's a reduced case I got out of llvm-reduce https://godbolt.org/z/WMTPcoe7z