[VectorCombine][RISCV] Convert VPIntrinsics with splat operands to splats #65706

michaelmaitland · 2023-09-08T03:16:02Z

of the scalar operation

VP Intrinsics whose vector operands are both splat values may be simplified into the scalar version of the operation and the result is splatted. If this simplification occurs, then it can lead to scalarization during CodeGen.

This issue is the intrinsic dual of #65072. This issue scalarizes non-legal types when the operations are VP Intrinsics.

michaelmaitland

I've pointed out a few cases that we may not want to do this optimization. Wondering if anyone has any feedback.

llvm/test/CodeGen/RISCV/rvv/vpbinops-scalarization.ll

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

llvm/test/CodeGen/RISCV/rvv/vpbinops-scalarization.ll

lukel97 · 2023-09-08T11:12:01Z

@ChunyuLiao pointed out that the VectorCombine pass already scalarizes binary ops with splatted operands at the IR level: https://godbolt.org/z/MPvvTG5dT

Would it make sense to do this in VectorCombine::scalarizeBinopOrCmp? It seems to have some cost modelling in there which I'd imagine would be good to take advantage of

michaelmaitland · 2023-09-08T19:38:50Z

Would it make sense to do this in VectorCombine::scalarizeBinopOrCmp? It seems to have some cost modelling in there which I'd imagine would be good to take advantage of

It isn't quite similar enough to fit right into VectorCombine::scalarizeBinopOrCmp, but I've put it in right around that same area. Having the cost model from VectorCombine there is a great idea.

lukel97 · 2023-09-11T16:34:53Z

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

+  // is a poison value. For now, only do this simplification if all lanes
+  // are active.
+  // TODO: Relax the condition that all lanes are active by using insertelement
+  // on inactive lanes.


Not for this patch, but for reinserting the inactive lanes later maybe we could do something like

%x = scalar %v = splat %res = @llvm.vp.merge.v16i32(%mask, %v, poison, %evl)

This is a good idea. I will work on this after this patch lands.

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

lukel97

This is looking pretty good. Just a note, not related to your patch, but about a missed scalarization in the existing non-vp scalarization: It only catches binops where both operands aren't constant, e.g. like this:

define <vscale x 1 x i64> @f(i64 %x, i64 %y) {
  %head.x = insertelement <vscale x 1 x i64> poison, i64 %x, i32 0
  %splat.x = shufflevector <vscale x 1 x i64> %head.x, <vscale x 1 x i64> poison, <vscale x 1 x i32> zeroinitializer
  %head.y = insertelement <vscale x 1 x i64> poison, i64 %y, i32 0
  %splat.y = shufflevector <vscale x 1 x i64> %head.y, <vscale x 1 x i64> poison, <vscale x 1 x i32> zeroinitializer
  %v = add <vscale x 1 x i64> %splat.x, %splat.y
  ret <vscale x 1 x i64> %v
}

Because this happens to get transformed by instcombine into:

define <vscale x 1 x i64> @f(i64 %x, i64 %y) #0 {
  %head.x = insertelement <vscale x 1 x i64> poison, i64 %x, i64 0
  %head.y = insertelement <vscale x 1 x i64> poison, i64 %y, i64 0
  %1 = add <vscale x 1 x i64> %head.x, %head.y
  %v = shufflevector <vscale x 1 x i64> %1, <vscale x 1 x i64> poison, <vscale x 1 x i32> zeroinitializer
  ret <vscale x 1 x i64> %v
}

And scalarizeBinopOrCmp only looks for insertelements.

But if one of the operands of the binop is a constant:

define <vscale x 1 x i64> @g(i64 %x) {
  %head.x = insertelement <vscale x 1 x i64> poison, i64 %x, i64 0
  %splat.x = shufflevector <vscale x 1 x i64> %head.x, <vscale x 1 x i64> poison, <vscale x 1 x i32> zeroinitializer
  %splat.y = shufflevector <vscale x 1 x i64> insertelement(<vscale x 1 x i64> poison, i64 42, i32 0), <vscale x 1 x i64> poison, <vscale x 1 x i32> zeroinitializer
  %v = add <vscale x 1 x i64> %splat.x, %splat.y
  ret <vscale x 1 x i64> %v
}

Then the above transformation doesn't happen, and it stays in the shufflevector %x, poison, zeroinitializer form. Which scalarizeBinopOrCmp doesn't handle.

llvm/test/Transforms/VectorCombine/RISCV/vpintrin-scalarization.ll

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

llvm/test/Transforms/VectorCombine/RISCV/vpintrin-scalarization.ll

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

lukel97 · 2023-09-13T16:59:07Z

Thank you very much for your review on this. You were very helpful in improving this patch and I learned a lot about a space I was not too familiar with previously.

No problem, I learnt lots about UB too :)

topperc · 2023-09-16T19:19:08Z

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

+    ScalarIntrID = VPI.getFunctionalIntrinsicID();
+    if (!ScalarIntrID)
+      return false;
+    ScalarIsIntr = true;


Do we need this flag? Can we check if ScalarIntrID has a value?

topperc · 2023-09-16T19:20:21Z

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

+  bool MustHaveNonZeroVL =
+      IntrID == Intrinsic::vp_sdiv || IntrID == Intrinsic::vp_udiv ||
+      IntrID == Intrinsic::vp_srem || IntrID == Intrinsic::vp_urem ||
+      IntrID == Intrinsic::vp_fdiv || IntrID  == Intrinsic::vp_frem;


fp isn't an issue. only integer.

Updated. Do you mind explaining why?

FP division by 0 produces infinity or negative infinity unless than numerator is 0, then it's NaN.

For integer division: The quotient of division by zero has all bits set, i.e. 2XLEN − 1 for unsigned division or −1 for signed division [Source, page 48] (https://riscv.org/wp-content/uploads/2017/05/riscv-spec-v2.2.pdf).

Why do we want to avoid doing integer division but okay with fp division?

This is not RISC-V specific code so the RISC-V spec does not apply. https://llvm.org/docs/LangRef.html#udiv-instruction "Division by zero is undefined behavior."

Ahh, I see, my bad. I've updated to remove check for fp here.

topperc · 2023-09-16T19:21:26Z

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

+      IntrID == Intrinsic::vp_srem || IntrID == Intrinsic::vp_urem ||
+      IntrID == Intrinsic::vp_fdiv || IntrID  == Intrinsic::vp_frem;
+
+  if ((MustHaveNonZeroVL && IsKnownNonZeroVL) || !MustHaveNonZeroVL) {


This is just !MustHaveNonZeroVL || IsKnownNonZeroVL

topperc · 2023-09-16T19:22:38Z

llvm/lib/Transforms/Vectorize/VectorCombine.cpp

+  ElementCount EC = cast<VectorType>(Op0->getType())->getElementCount();
+  Value *EVL = VPI.getArgOperand(3);
+  const DataLayout &DL = VPI.getModule()->getDataLayout();
+  bool IsKnownNonZeroVL = isKnownNonZero(EVL, DL, 0, &AC, &VPI, &DT);


We should only call isKnownNonZero if we need it. It's expensive.

of the scalar operation VP Intrinsics whose vector operands are both splat values may be simplified into the scalar version of the operation and the result is splatted. If this simplification occurs, then it can lead to scalarization during CodeGen. This issue is the intrinsic dual of llvm#65072. This issue scalarizes non-legal types when the operations are VP Intrinsics.

…to splats of the scalar operation

…to splats of the scalar operation Use getFunctionalIntrinsicID

…to splats of the scalar operation Add zvfh and VEC-COMBINE-64/32

…to splats of the scalar operation Respond to craigs comments

…lvm#66190) This adds a helper method to get the ID of the functionally equivalent intrinsic, similar to the existing getFunctionalOpcodeForVP and getConstrainedIntrinsicIDForVP methods. Not sure if it's notable or not, but I can't find any existing uses of VP_PROPERTY_FUNCTIONAL_INTRINSIC? It could potentially be used in llvm#65706 to scalarize VP intrinsics.

VPIntrinsics with VP_PROPERTY_BINARYOP property should have the ability to be queried with with VPBinOpIntrinsic::isVPBinOp, similiar to how intrinsics with the VP_PROPERTY_REDUCTION property can be queried with VPReductionIntrinsic::isVPReduction. This will be used in llvm#65706. In that PR the usage of this class is tested.

topperc

LGTM

This directory was missing a lit.local.cfg which was causing some build bots to fail when #65706 was comitted.

michaelmaitland added backend:RISC-V llvm:instcombine labels Sep 8, 2023

michaelmaitland requested review from preames, lukel97 and topperc September 8, 2023 03:16

michaelmaitland requested review from a team as code owners September 8, 2023 03:16

michaelmaitland changed the title ~~[InstCombine][RISCV] Convert VPIntrinsics with splat operands to splats~~ [InstCombine][RISCV] Convert VPIntrinsics with splat operands to splats of scalar operand Sep 8, 2023

michaelmaitland changed the title ~~[InstCombine][RISCV] Convert VPIntrinsics with splat operands to splats of scalar operand~~ [InstCombine][RISCV] Convert VPIntrinsics with splat operands to splats Sep 8, 2023

michaelmaitland commented Sep 8, 2023

View reviewed changes

topperc reviewed Sep 8, 2023

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp Outdated Show resolved Hide resolved

topperc reviewed Sep 8, 2023

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp Outdated Show resolved Hide resolved

topperc reviewed Sep 8, 2023

View reviewed changes

llvm/test/CodeGen/RISCV/rvv/vpbinops-scalarization.ll Outdated Show resolved Hide resolved

michaelmaitland force-pushed the vpbinops-scalarization branch from f118ff1 to d8b25ea Compare September 8, 2023 03:50

github-actions bot added the vectorizers label Sep 8, 2023

michaelmaitland force-pushed the vpbinops-scalarization branch 3 times, most recently from 823e887 to 95d9fe3 Compare September 8, 2023 21:19

github-actions bot added vectorizers llvm:transforms labels Sep 8, 2023

michaelmaitland force-pushed the vpbinops-scalarization branch from 2150a6d to 1cfeeec Compare September 11, 2023 13:44

lukel97 reviewed Sep 11, 2023

View reviewed changes

llvm/lib/Transforms/Vectorize/VectorCombine.cpp Show resolved Hide resolved

lukel97 reviewed Sep 12, 2023

View reviewed changes

llvm/test/Transforms/VectorCombine/RISCV/vpintrin-scalarization.ll Outdated Show resolved Hide resolved

llvm/test/Transforms/VectorCombine/RISCV/vpintrin-scalarization.ll Show resolved Hide resolved

lukel97 reviewed Sep 12, 2023

View reviewed changes

llvm/lib/Transforms/Vectorize/VectorCombine.cpp Outdated Show resolved Hide resolved

michaelmaitland force-pushed the vpbinops-scalarization branch from 7eae810 to 31d1880 Compare September 12, 2023 15:44

michaelmaitland commented Sep 12, 2023

View reviewed changes

llvm/test/Transforms/VectorCombine/RISCV/vpintrin-scalarization.ll Show resolved Hide resolved

lukel97 reviewed Sep 12, 2023

View reviewed changes

llvm/lib/Transforms/Vectorize/VectorCombine.cpp Outdated Show resolved Hide resolved

michaelmaitland requested a review from RKSimon September 13, 2023 16:53

nikic changed the title ~~[InstCombine][RISCV] Convert VPIntrinsics with splat operands to splats~~ [VectorCombine][RISCV] Convert VPIntrinsics with splat operands to splats Sep 14, 2023

michaelmaitland removed the llvm:instcombine label Sep 14, 2023

michaelmaitland force-pushed the vpbinops-scalarization branch from 949275c to 256f2ad Compare September 14, 2023 14:01

topperc reviewed Sep 16, 2023

View reviewed changes

michaelmaitland force-pushed the vpbinops-scalarization branch from 408253e to be0847f Compare September 18, 2023 14:10

michaelmaitland added 14 commits September 18, 2023 15:59

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

618349b

…to splats of the scalar operation

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

b843089

…to splats of the scalar operation

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

52fb71e

…to splats of the scalar operation

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

adab8fa

…to splats of the scalar operation

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

6efc815

…to splats of the scalar operation

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

8c23455

…to splats of the scalar operation

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

2cda6cb

…to splats of the scalar operation

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

aefb961

…to splats of the scalar operation

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

8b30ae5

…to splats of the scalar operation

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

6abb0d3

…to splats of the scalar operation

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

ca48343

…to splats of the scalar operation Use getFunctionalIntrinsicID

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

2b96e3f

…to splats of the scalar operation Add zvfh and VEC-COMBINE-64/32

fixup! [InstCombine][RISCV] Convert VPIntrinsics with splat operands …

5cc6e53

…to splats of the scalar operation Respond to craigs comments

michaelmaitland force-pushed the vpbinops-scalarization branch from be0847f to 5cc6e53 Compare September 18, 2023 23:00

topperc approved these changes Sep 20, 2023

View reviewed changes

michaelmaitland merged commit e0aaa19 into llvm:main Sep 20, 2023

michaelmaitland deleted the vpbinops-scalarization branch September 20, 2023 22:27

michaelmaitland added a commit that referenced this pull request Sep 20, 2023

[RISCV] Add llvm/test/Transforms/VectorCombine/RISCV/lit.local.cfg

81b0c24

This directory was missing a lit.local.cfg which was causing some build bots to fail when #65706 was comitted.

kstoimenov mentioned this pull request Sep 22, 2023

Add memcpm test kstoimenov/llvm-project#13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VectorCombine][RISCV] Convert VPIntrinsics with splat operands to splats #65706

[VectorCombine][RISCV] Convert VPIntrinsics with splat operands to splats #65706

michaelmaitland commented Sep 8, 2023

michaelmaitland left a comment

lukel97 commented Sep 8, 2023

michaelmaitland commented Sep 8, 2023

lukel97 Sep 11, 2023

michaelmaitland Sep 11, 2023

lukel97 left a comment •

edited

Loading

lukel97 commented Sep 13, 2023

topperc Sep 16, 2023

michaelmaitland Sep 18, 2023

topperc Sep 16, 2023

michaelmaitland Sep 18, 2023

topperc Sep 18, 2023

michaelmaitland Sep 18, 2023

topperc Sep 18, 2023

michaelmaitland Sep 18, 2023

topperc Sep 16, 2023

michaelmaitland Sep 18, 2023

topperc Sep 16, 2023

michaelmaitland Sep 18, 2023

topperc left a comment

[VectorCombine][RISCV] Convert VPIntrinsics with splat operands to splats #65706

[VectorCombine][RISCV] Convert VPIntrinsics with splat operands to splats #65706

Conversation

michaelmaitland commented Sep 8, 2023

michaelmaitland left a comment

Choose a reason for hiding this comment

lukel97 commented Sep 8, 2023

michaelmaitland commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukel97 left a comment • edited Loading

Choose a reason for hiding this comment

lukel97 commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topperc left a comment

Choose a reason for hiding this comment

lukel97 left a comment •

edited

Loading