[LV][EVL]Support reversed loads/stores.

[alexey-bataev]

Support for predicated vector reverse intrinsic was added some time ago.
Adds support for predicated reversed loads/stores in the loop
vectorizer.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Alexey Bataev (alexey-bataev)

Changes

Support for predicated vector reverse intrinsic was added some time ago.
Adds support for predicated reversed loads/stores in the loop
vectorizer.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+45-22)
• (modified) llvm/test/Transforms/LoopVectorize/RISCV/vectorize-force-tail-with-evl-reverse-load-store.ll (+10-13)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 9e22dce384773e..797a3fd1e9dba4 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -1579,13 +1579,7 @@ class LoopVectorizationCostModel {
   /// Returns true if VP intrinsics with explicit vector length support should
   /// be generated in the tail folded loop.
   bool foldTailWithEVL() const {
-    return getTailFoldingStyle() == TailFoldingStyle::DataWithEVL &&
-           // FIXME: remove this once vp_reverse is supported.
-           none_of(
-               WideningDecisions,
-               [](const std::pair<std::pair<Instruction *, ElementCount>,
-                                  std::pair<InstWidening, InstructionCost>>
-                      &Data) { return Data.second.first == CM_Widen_Reverse; });
+    return getTailFoldingStyle() == TailFoldingStyle::DataWithEVL;
   }
 
   /// Returns true if the Phi is part of an inloop reduction.
@@ -9361,10 +9355,17 @@ void VPReplicateRecipe::execute(VPTransformState &State) {
 
 /// Creates either vp_store or vp_scatter intrinsics calls to represent
 /// predicated store/scatter.
-static Instruction *
-lowerStoreUsingVectorIntrinsics(IRBuilderBase &Builder, Value *Addr,
-                                Value *StoredVal, bool IsScatter, Value *Mask,
-                                Value *EVL, const Align &Alignment) {
+static Instruction *lowerStoreUsingVectorIntrinsics(
+    IRBuilderBase &Builder, Value *Addr, Value *StoredVal, bool IsScatter,
+    bool IsReverse, Value *Mask, Value *EVL, const Align &Alignment) {
+  if (IsReverse) {
+    auto *StoredValTy = cast<VectorType>(StoredVal->getType());
+    Value *BlockInMaskPart =
+        Builder.getAllOnesMask(StoredValTy->getElementCount());
+    StoredVal = Builder.CreateIntrinsic(
+        StoredValTy, Intrinsic::experimental_vp_reverse,
+        {StoredVal, BlockInMaskPart, EVL}, nullptr, "vp.reverse");
+  }
   CallInst *Call;
   if (IsScatter) {
     Call = Builder.CreateIntrinsic(Type::getVoidTy(EVL->getContext()),
@@ -9384,11 +9385,9 @@ lowerStoreUsingVectorIntrinsics(IRBuilderBase &Builder, Value *Addr,
 
 /// Creates either vp_load or vp_gather intrinsics calls to represent
 /// predicated load/gather.
-static Instruction *lowerLoadUsingVectorIntrinsics(IRBuilderBase &Builder,
-                                                   VectorType *DataTy,
-                                                   Value *Addr, bool IsGather,
-                                                   Value *Mask, Value *EVL,
-                                                   const Align &Alignment) {
+static Instruction *lowerLoadUsingVectorIntrinsics(
+    IRBuilderBase &Builder, VectorType *DataTy, Value *Addr, bool IsGather,
+    bool IsReverse, Value *Mask, Value *EVL, const Align &Alignment) {
   CallInst *Call;
   if (IsGather) {
     Call =
@@ -9402,7 +9401,14 @@ static Instruction *lowerLoadUsingVectorIntrinsics(IRBuilderBase &Builder,
   }
   Call->addParamAttr(
       0, Attribute::getWithAlignment(Call->getContext(), Alignment));
-  return Call;
+  Instruction *Res = Call;
+  if (IsReverse) {
+    Value *BlockInMaskPart = Builder.getAllOnesMask(DataTy->getElementCount());
+    Res = Builder.CreateIntrinsic(DataTy, Intrinsic::experimental_vp_reverse,
+                                  {Res, BlockInMaskPart, EVL}, nullptr,
+                                  "vp.reverse");
+  }
+  return Res;
 }
 
 void VPWidenMemoryInstructionRecipe::execute(VPTransformState &State) {
@@ -9430,7 +9436,7 @@ void VPWidenMemoryInstructionRecipe::execute(VPTransformState &State) {
     // a null all-one mask is a null mask.
     for (unsigned Part = 0; Part < State.UF; ++Part) {
       Value *Mask = State.get(getMask(), Part);
-      if (isReverse())
+      if (isReverse() && !State.EVL)
         Mask = Builder.CreateVectorReverse(Mask, "reverse");
       BlockInMaskParts[Part] = Mask;
     }
@@ -9456,11 +9462,20 @@ void VPWidenMemoryInstructionRecipe::execute(VPTransformState &State) {
         // is created only if TTI prefers predicated vectorization, thus if EVL
         // is not nullptr it also implies preference for predicated
         // vectorization.
-        // FIXME: Support reverse store after vp_reverse is added.
         Value *MaskPart = isMaskRequired ? BlockInMaskParts[Part] : nullptr;
+        if (isMaskRequired && isReverse() && !getMask()->isLiveIn()) {
+          VectorType *MaskTy = cast<VectorType>(MaskPart->getType());
+          Value *BlockInMaskPart =
+              Builder.getAllOnesMask(MaskTy->getElementCount());
+          MaskPart = Builder.CreateIntrinsic(
+              MaskTy, Intrinsic::experimental_vp_reverse,
+              {MaskPart, BlockInMaskPart, EVL}, nullptr, "vp.reverse.mask");
+          BlockInMaskParts[Part] = MaskPart;
+        }
         NewSI = lowerStoreUsingVectorIntrinsics(
             Builder, State.get(getAddr(), Part, !CreateGatherScatter),
-            StoredVal, CreateGatherScatter, MaskPart, EVL, Alignment);
+            StoredVal, CreateGatherScatter, isReverse(), MaskPart, EVL,
+            Alignment);
       } else if (CreateGatherScatter) {
         Value *MaskPart = isMaskRequired ? BlockInMaskParts[Part] : nullptr;
         Value *VectorGep = State.get(getAddr(), Part);
@@ -9504,11 +9519,19 @@ void VPWidenMemoryInstructionRecipe::execute(VPTransformState &State) {
       // is created only if TTI prefers predicated vectorization, thus if EVL
       // is not nullptr it also implies preference for predicated
       // vectorization.
-      // FIXME: Support reverse loading after vp_reverse is added.
       Value *MaskPart = isMaskRequired ? BlockInMaskParts[Part] : nullptr;
+      if (isMaskRequired && isReverse() && !getMask()->isLiveIn()) {
+        VectorType *MaskTy = cast<VectorType>(MaskPart->getType());
+        Value *BlockInMaskPart =
+            Builder.getAllOnesMask(MaskTy->getElementCount());
+        MaskPart = Builder.CreateIntrinsic(
+            MaskTy, Intrinsic::experimental_vp_reverse,
+            {MaskPart, BlockInMaskPart, EVL}, nullptr, "vp.reverse.mask");
+        BlockInMaskParts[Part] = MaskPart;
+      }
       NewLI = lowerLoadUsingVectorIntrinsics(
           Builder, DataTy, State.get(getAddr(), Part, !CreateGatherScatter),
-          CreateGatherScatter, MaskPart, EVL, Alignment);
+          CreateGatherScatter, isReverse(), MaskPart, EVL, Alignment);
     } else if (CreateGatherScatter) {
       Value *MaskPart = isMaskRequired ? BlockInMaskParts[Part] : nullptr;
       Value *VectorGep = State.get(getAddr(), Part);
diff --git a/llvm/test/Transforms/LoopVectorize/RISCV/vectorize-force-tail-with-evl-reverse-load-store.ll b/llvm/test/Transforms/LoopVectorize/RISCV/vectorize-force-tail-with-evl-reverse-load-store.ll
index f2222e0a1f936a..f839eafe9b2a61 100644
--- a/llvm/test/Transforms/LoopVectorize/RISCV/vectorize-force-tail-with-evl-reverse-load-store.ll
+++ b/llvm/test/Transforms/LoopVectorize/RISCV/vectorize-force-tail-with-evl-reverse-load-store.ll
@@ -30,14 +30,11 @@ define void @reverse_load_store(i64 %startval, ptr noalias %ptr, ptr noalias %pt
 ; IF-EVL-NEXT:    br label [[VECTOR_BODY:%.*]]
 ; IF-EVL:       vector.body:
 ; IF-EVL-NEXT:    [[INDEX:%.*]] = phi i64 [ 0, [[VECTOR_PH]] ], [ [[INDEX_NEXT:%.*]], [[VECTOR_BODY]] ]
-; IF-EVL-NEXT:    [[OFFSET_IDX:%.*]] = sub i64 [[STARTVAL]], [[INDEX]]
+; IF-EVL-NEXT:    [[EVL_BASED_IV:%.*]] = phi i64 [ 0, [[VECTOR_PH]] ], [ [[INDEX_EVL_NEXT:%.*]], [[VECTOR_BODY]] ]
+; IF-EVL-NEXT:    [[TMP9:%.*]] = sub i64 1024, [[EVL_BASED_IV]]
+; IF-EVL-NEXT:    [[TMP8:%.*]] = call i32 @llvm.experimental.get.vector.length.i64(i64 [[TMP9]], i32 4, i1 true)
+; IF-EVL-NEXT:    [[OFFSET_IDX:%.*]] = sub i64 [[STARTVAL]], [[EVL_BASED_IV]]
 ; IF-EVL-NEXT:    [[TMP7:%.*]] = add i64 [[OFFSET_IDX]], 0
-; IF-EVL-NEXT:    [[BROADCAST_SPLATINSERT:%.*]] = insertelement <vscale x 4 x i64> poison, i64 [[INDEX]], i64 0
-; IF-EVL-NEXT:    [[BROADCAST_SPLAT:%.*]] = shufflevector <vscale x 4 x i64> [[BROADCAST_SPLATINSERT]], <vscale x 4 x i64> poison, <vscale x 4 x i32> zeroinitializer
-; IF-EVL-NEXT:    [[TMP8:%.*]] = call <vscale x 4 x i64> @llvm.experimental.stepvector.nxv4i64()
-; IF-EVL-NEXT:    [[TMP9:%.*]] = add <vscale x 4 x i64> zeroinitializer, [[TMP8]]
-; IF-EVL-NEXT:    [[VEC_IV:%.*]] = add <vscale x 4 x i64> [[BROADCAST_SPLAT]], [[TMP9]]
-; IF-EVL-NEXT:    [[TMP10:%.*]] = icmp ule <vscale x 4 x i64> [[VEC_IV]], shufflevector (<vscale x 4 x i64> insertelement (<vscale x 4 x i64> poison, i64 1023, i64 0), <vscale x 4 x i64> poison, <vscale x 4 x i32> zeroinitializer)
 ; IF-EVL-NEXT:    [[TMP11:%.*]] = add i64 [[TMP7]], -1
 ; IF-EVL-NEXT:    [[TMP12:%.*]] = getelementptr inbounds i32, ptr [[PTR:%.*]], i64 [[TMP11]]
 ; IF-EVL-NEXT:    [[TMP13:%.*]] = call i64 @llvm.vscale.i64()
@@ -46,9 +43,8 @@ define void @reverse_load_store(i64 %startval, ptr noalias %ptr, ptr noalias %pt
 ; IF-EVL-NEXT:    [[TMP16:%.*]] = sub i64 1, [[TMP14]]
 ; IF-EVL-NEXT:    [[TMP17:%.*]] = getelementptr inbounds i32, ptr [[TMP12]], i64 [[TMP15]]
 ; IF-EVL-NEXT:    [[TMP18:%.*]] = getelementptr inbounds i32, ptr [[TMP17]], i64 [[TMP16]]
-; IF-EVL-NEXT:    [[REVERSE:%.*]] = call <vscale x 4 x i1> @llvm.experimental.vector.reverse.nxv4i1(<vscale x 4 x i1> [[TMP10]])
-; IF-EVL-NEXT:    [[WIDE_MASKED_LOAD:%.*]] = call <vscale x 4 x i32> @llvm.masked.load.nxv4i32.p0(ptr [[TMP18]], i32 4, <vscale x 4 x i1> [[REVERSE]], <vscale x 4 x i32> poison)
-; IF-EVL-NEXT:    [[REVERSE3:%.*]] = call <vscale x 4 x i32> @llvm.experimental.vector.reverse.nxv4i32(<vscale x 4 x i32> [[WIDE_MASKED_LOAD]])
+; IF-EVL-NEXT:    [[VP_OP_LOAD:%.*]] = call <vscale x 4 x i32> @llvm.vp.load.nxv4i32.p0(ptr align 4 [[TMP18]], <vscale x 4 x i1> shufflevector (<vscale x 4 x i1> insertelement (<vscale x 4 x i1> poison, i1 true, i64 0), <vscale x 4 x i1> poison, <vscale x 4 x i32> zeroinitializer), i32 [[TMP8]])
+; IF-EVL-NEXT:    [[TMP31:%.*]] = call <vscale x 4 x i32> @llvm.experimental.vp.reverse.nxv4i32(<vscale x 4 x i32> [[VP_OP_LOAD]], <vscale x 4 x i1> shufflevector (<vscale x 4 x i1> insertelement (<vscale x 4 x i1> poison, i1 true, i64 0), <vscale x 4 x i1> poison, <vscale x 4 x i32> zeroinitializer), i32 [[TMP8]])
 ; IF-EVL-NEXT:    [[TMP19:%.*]] = getelementptr inbounds i32, ptr [[PTR2:%.*]], i64 [[TMP11]]
 ; IF-EVL-NEXT:    [[TMP20:%.*]] = call i64 @llvm.vscale.i64()
 ; IF-EVL-NEXT:    [[TMP21:%.*]] = mul i64 [[TMP20]], 4
@@ -56,9 +52,10 @@ define void @reverse_load_store(i64 %startval, ptr noalias %ptr, ptr noalias %pt
 ; IF-EVL-NEXT:    [[TMP23:%.*]] = sub i64 1, [[TMP21]]
 ; IF-EVL-NEXT:    [[TMP24:%.*]] = getelementptr inbounds i32, ptr [[TMP19]], i64 [[TMP22]]
 ; IF-EVL-NEXT:    [[TMP25:%.*]] = getelementptr inbounds i32, ptr [[TMP24]], i64 [[TMP23]]
-; IF-EVL-NEXT:    [[REVERSE4:%.*]] = call <vscale x 4 x i1> @llvm.experimental.vector.reverse.nxv4i1(<vscale x 4 x i1> [[TMP10]])
-; IF-EVL-NEXT:    [[REVERSE5:%.*]] = call <vscale x 4 x i32> @llvm.experimental.vector.reverse.nxv4i32(<vscale x 4 x i32> [[REVERSE3]])
-; IF-EVL-NEXT:    call void @llvm.masked.store.nxv4i32.p0(<vscale x 4 x i32> [[REVERSE5]], ptr [[TMP25]], i32 4, <vscale x 4 x i1> [[REVERSE4]])
+; IF-EVL-NEXT:    [[TMP28:%.*]] = call <vscale x 4 x i32> @llvm.experimental.vp.reverse.nxv4i32(<vscale x 4 x i32> [[TMP31]], <vscale x 4 x i1> shufflevector (<vscale x 4 x i1> insertelement (<vscale x 4 x i1> poison, i1 true, i64 0), <vscale x 4 x i1> poison, <vscale x 4 x i32> zeroinitializer), i32 [[TMP8]])
+; IF-EVL-NEXT:    call void @llvm.vp.store.nxv4i32.p0(<vscale x 4 x i32> [[TMP28]], ptr align 4 [[TMP25]], <vscale x 4 x i1> shufflevector (<vscale x 4 x i1> insertelement (<vscale x 4 x i1> poison, i1 true, i64 0), <vscale x 4 x i1> poison, <vscale x 4 x i32> zeroinitializer), i32 [[TMP8]])
+; IF-EVL-NEXT:    [[TMP29:%.*]] = zext i32 [[TMP8]] to i64
+; IF-EVL-NEXT:    [[INDEX_EVL_NEXT]] = add i64 [[TMP29]], [[EVL_BASED_IV]]
 ; IF-EVL-NEXT:    [[INDEX_NEXT]] = add i64 [[INDEX]], [[TMP6]]
 ; IF-EVL-NEXT:    [[TMP26:%.*]] = icmp eq i64 [[INDEX_NEXT]], [[N_VEC]]
 ; IF-EVL-NEXT:    br i1 [[TMP26]], label [[MIDDLE_BLOCK:%.*]], label [[VECTOR_BODY]], !llvm.loop [[LOOP0:![0-9]+]]

[alexey-bataev]

Ping!

[alexey-bataev]

Ping!

[alexey-bataev]

Ping!

[alexey-bataev]

Ping!

[fhahn]

Thanks for the rebase!

[fhahn]

From looking at the test changes, it seems like this patch may not be covered?

[fhahn]

The newly added test only seems to cover stores with masks, could you add one with loads that need a mask as well?

[fhahn]

LGTM, but please move the code to create vp.reverse with all-true mask to a static helper to reduce duplication and make sure that it is done consistently.

[ayalz]

Unclear if/when "masked reverse" should be used, when reversing masked operands.

[alexey-bataev]

Ping!

[alexey-bataev]

Ping!

[ayalz]

Suggested change

	static Value *reverseMask(IRBuilderBase &Builder, Value *Mask, Value *EVL,
	static Value *createReverseMaskEVL(IRBuilderBase &Builder, Value *Mask, Value *EVL,

[ayalz]

Should the above reverse of the loaded values use the partial mask of these values, rather than an all-true mask?

Should the above reverse of the to-be-stored values use the partial mask of these values, rather than an all-true mask?

Not necessary, all-true is good enough here, real mask just increases pressure on mask register with no effect

Worth a comment that the real mask is being optimized away here by a cheaper AllTrueMask.

As noted, it's "Unclear if/when "masked reverse" should be used, when reversing masked operands", i.e.,
where - for which reverses, if any? - all-true would not be good enough.

Would it be better to have createReverseEVL(IRBuilderBase &Builder, Value *Operand, Value *EVL, const Twine &Name) generate an experimental_vp_reverse for masks and values alike, also filling-in the optimized-out AllTrueMask?

[alexey-bataev]

As noted, it's "Unclear if/when "masked reverse" should be used, when reversing masked operands", i.e., where - for which reverses, if any? - all-true would not be good enough.

The reversing of the values in registers is safe (the actual length of the reversed data is controlled by EVL, we just cut extra unused lanes, because of that we can safely use all-true mask). Does it answer your question?

2. Added createReverseEVL and used it.

[ayalz]

It's obviously safe for reverse to utilize an all-true mask - as does the non-EVL vectorizer. Question was if it is ever beneficial not to do so? I.e., if in practice the mask of reverse is an unused operand that only wastes a register.

[fhahn]

LGTM with a few additional nits inline.

Looks like all comments have been addressed, thanks!

[ayalz]

It's obviously safe for reverse to utilize an all-true mask - as does the non-EVL vectorizer. Question was if it is ever beneficial not to do so? I.e., if in practice the mask of reverse is an unused operand that only wastes a register.

[alexey-bataev]

It's obviously safe for reverse to utilize an all-true mask - as does the non-EVL vectorizer. Question was if it is ever beneficial not to do so? I.e., if in practice the mask of reverse is an unused operand that only wastes a register.

Yes, always better from our perf data. Reducing dependencies increases the throughput, allows to execute instructions in parallel.

[ayalz]

LGTM with a few additional nits inline.

Looks like all comments have been addressed, thanks!

Added a minor nit (post-commit): explanation that an all-true mask is always used for reverse is better placed when documenting createReverseEVL(), rather than (some of) its callers.