Fix for bug 41512: lower INSERT_VECTOR_ELT(ZeroVec, 0, Elt) to SCALAR_TO_VECTOR(Elt) for all SSE flavors

[Serge_Preis]

Summary

Current LLVM uses pxor+pinsrb on SSE4+ for INSERT_VECTOR_ELT(ZeroVec, 0, Elt) insead of much simpler movd.
INSERT_VECTOR_ELT(ZeroVec, 0, Elt) is idiomatic construct which is used e.g. for _mm_cvtsi32_si128(Elt) and for lowest element initialization in _mm_set_epi32.
So such inefficient lowering leads to significant performance digradations in ceratin cases switching from SSSE3 to SSE4.
https://bugs.llvm.org/show_bug.cgi?id=41512

Here INSERT_VECTOR_ELT(ZeroVec, 0, Elt) is simply converted to SCALAR_TO_VECTOR(Elt) when applicable since latter is closer match to desired behavior and always efficiently lowered to movd and alike.

[RKSimon]

Your test cases need to be a lot simpler - I'd recommend looking at buildvec-insertvec.ll and possibly adding your tests to that file instead of adding this new file.

You will need to use the utils/update_llc_test_checks.py script to autogenerate your codegen checks as well.

[Serge_Preis]

In D60852#1471378, @RKSimon wrote:

Your test cases need to be a lot simpler - I'd recommend looking at buildvec-insertvec.ll and possibly adding your tests to that file instead of adding this new file.

The problem is that in tiny kernel llvm behaves as I expect it to, while in a loop it underperforms: https://gcc.godbolt.org/z/PljujX -- compare Sum2 and Loop() code generation.
I will do my best to minimize the case.

You will need to use the utils/update_llc_test_checks.py script to autogenerate your codegen checks as well.

Ok, I will look into this.

[spatel]

In D60852#1471492, @Serge_Preis wrote:

In D60852#1471378, @RKSimon wrote:

Your test cases need to be a lot simpler - I'd recommend looking at buildvec-insertvec.ll and possibly adding your tests to that file instead of adding this new file.

The problem is that in tiny kernel llvm behaves as I expect it to, while in a loop it underperforms: https://gcc.godbolt.org/z/PljujX -- compare Sum2 and Loop() code generation.
I will do my best to minimize the case.

If we are getting this right sometimes, then we might already have the transform that we want, but it is limited in some way that prevents getting the larger case.

I doubt that the loop itself is needed to demonstrate the problem because I see 'movd' codegen even with a loop as long as it is not unrolled.

[Serge_Preis]

If we are getting this right sometimes, then we might already have the transform that we want, but it is limited in some way that prevents getting the larger case.
I doubt that the loop itself is needed to demonstrate the problem because I see 'movd' codegen even with a loop as long as it is not unrolled.

After more experimentation I tend to agree. Also the most basic case produces pinsrd even in a small kernel (https://gcc.godbolt.org/z/HAmNha), so will just create test out of it.

[spatel]

In D60852#1471661, @Serge_Preis wrote:

If we are getting this right sometimes, then we might already have the transform that we want, but it is limited in some way that prevents getting the larger case.
I doubt that the loop itself is needed to demonstrate the problem because I see 'movd' codegen even with a loop as long as it is not unrolled.

After more experimentation I tend to agree. Also the most basic case produces pinsrd even in a small kernel (https://gcc.godbolt.org/z/HAmNha), so will just create test out of it.

Not sure if this is minimal, but this seems to show the problem:

define <2 x i64> @pinsr(i32 %x, i32 %y) {
  %ins1 = insertelement <4 x i32> <i32 undef, i32 0, i32 undef, i32 undef>, i32 %x, i32 0
  %ins2 = insertelement <4 x i32> <i32 undef, i32 0, i32 undef, i32 undef>, i32 %y, i32 0
  %b1 = bitcast <4 x i32> %ins1 to <2 x i64>
  %b2 = bitcast <4 x i32> %ins2 to <2 x i64>
  %r = shufflevector <2 x i64> %b1, <2 x i64> %b2, <2 x i32> <i32 0, i32 2>
  ret <2 x i64> %r
}

$ llc -o - pinsr.ll -mattr=sse4.2
	pxor	%xmm1, %xmm1
	pxor	%xmm0, %xmm0
	pinsrd	$0, %edi, %xmm0
	pinsrd	$0, %esi, %xmm1
	punpcklqdq	%xmm1, %xmm0    ## xmm0 = xmm0[0],xmm1[0]
	retq

[Serge_Preis]

Fixed test according to review comments

[spatel]

I added some baseline tests here:
rL358687

Please rebase/update against those and upload a new patch.

[Serge_Preis]

With updated insertelement-zero.ll test

[spatel]

This looks like the expected improvement to me, but given the 'blend' comment/test, I'd like someone else to confirm that I'm not overlooking some weird corner case.

llvm/test/CodeGen/X86/insertelement-zero.ll

503 ↗

(On Diff #195795)

Remove 'FIXME' from this line.

633–635 ↗

(On Diff #195795)

I looked at this case, and it is intentional because there's a comment in X86InstrSSE.td:
"these were changed to use blends because blends have better throughput on sandybridge and haswell"

[Serge_Preis]

Deleted FIXME

[Serge_Preis]

llvm/test/CodeGen/X86/insertelement-zero.ll

633–635 ↗

(On Diff #195795)

My change itself doen't dictate any particular instruction selection. it just makes more obvious to ISel the idiom it deals with. If some instructions are better than others for 1st element intialization they should be naturaly used after the fix. So I'm just happy to see confirmation here.

[Serge_Preis]

Friendly ping: how should I proceed with this review?

[RKSimon]

llvm/lib/Target/X86/X86ISelLowering.cpp

16972

Subtarget.is64Bit() should be superfluous

[Serge_Preis]

llvm/lib/Target/X86/X86ISelLowering.cpp

16972

This is a good question, On one (conservative) hand I am aiming at movq instruction, whch is only available on 64bit subtarget (and similar places have same set of checks). On other hand the logic behind the conversion is pretty generic and doesn't depend on subtarget.. So the question is "are we sure that 64bit SCALAR_TO_VECTOR will be handled efficiently now on 32bit subtarget or such generalization will require further patching?"

[craig.topper]

llvm/lib/Target/X86/X86ISelLowering.cpp

16972

Type legalization would have guaranteed there are no i64 values in the DAG if this is not a 64-bit target before we get to this code.

[Serge_Preis]

Removed subtarget check in response to review comments

[Serge_Preis]

llvm/lib/Target/X86/X86ISelLowering.cpp

16972

Ok. uploaded new iteration without subtarget check

[RKSimon]

LGTM - @Serge_Preis do you have commit access?

[Serge_Preis]

In D60852#1483730, @RKSimon wrote:

@Serge_Preis do you have commit access?

No, unfortunately don't.

llvm/lib/Target/X86/X86ISelLowering.cpp

if (VT.is256BitVector() || VT.is512BitVector()) {
     V = DAG.getNode(ISD::INSERT_VECTOR_ELT, dl, V.getValueType(), V, N1,
                     DAG.getConstant(IdxIn128, dl, MVT::i32));
 
     // Insert the changed part back into the bigger vector
     return insert128BitVector(N0, V, IdxVal, DAG, dl);
   }
   assert(VT.is128BitVector() && "Only 128-bit vector types should be left!");
 
+  // This will be just movd/movq/movss/movsd
+  if (IdxVal == 0 && ISD::isBuildVectorAllZeros(N0.getNode())) {
+    if (EltVT == MVT::i32 || EltVT == MVT::f32 || EltVT == MVT::f64 ||
+        (EltVT == MVT::i64 && Subtarget.is64Bit())) {
+      N1 = DAG.getNode(ISD::SCALAR_TO_VECTOR, dl, VT, N1);
+      return getShuffleVectorZeroOrUndef(N1, 0, true, Subtarget, DAG);
+    }
+  }
+
   // Transform it so it match pinsr{b,w} which expects a GR32 as its second
   // argument. SSE41 required for pinsrb.
   if (VT == MVT::v8i16 || (VT == MVT::v16i8 && Subtarget.hasSSE41())) {
     unsigned Opc;
     if (VT == MVT::v8i16) {
       assert(Subtarget.hasSSE2() && "SSE2 required for PINSRW");
       Opc = X86ISD::PINSRW;
     } else {

llvm/test/CodeGen/X86/vec_insert_first.ll

+; RUN: llc < %s -mtriple=i386-unknown -mattr=+sse4.1 | FileCheck %s
+; RUN: llc < %s -mtriple=x86_64-unknown -mattr=+sse4.1 | FileCheck %s
+
+define dso_local void @_Z4LoopPKiRDv2_xjj(i32* nocapture readonly %d, <2 x i64>* nocapture dereferenceable(16) %res, i32 %st1, i32 %st2) local_unnamed_addr #0 {
+
+; CHECK-NOT: pinsr
+entry:
+  %idxprom.i = zext i32 %st1 to i64
+  %arrayidx.i = getelementptr inbounds i32, i32* %d, i64 %idxprom.i
+  %idxprom1.i = zext i32 %st2 to i64
+  %arrayidx2.i = getelementptr inbounds i32, i32* %d, i64 %idxprom1.i
+  %0 = load i32, i32* %arrayidx.i, align 4
+  %vecinit3.i.i = insertelement <4 x i32> <i32 undef, i32 0, i32 undef, i32 undef>, i32 %0, i32 0
+  %1 = bitcast <4 x i32> %vecinit3.i.i to <2 x i64>
+  %2 = load i32, i32* %arrayidx2.i, align 4
+  %vecinit3.i10.i = insertelement <4 x i32> <i32 undef, i32 0, i32 undef, i32 undef>, i32 %2, i32 0
+  %3 = bitcast <4 x i32> %vecinit3.i10.i to <2 x i64>
+  %4 = load <2 x i64>, <2 x i64>* %res, align 16
+  %shuffle.i.i = shufflevector <2 x i64> %1, <2 x i64> %3, <2 x i32> <i32 0, i32 2>
+  %add.i.i = add <2 x i64> %shuffle.i.i, %4
+  store <2 x i64> %add.i.i, <2 x i64>* %res, align 16
+  %5 = load i32, i32* %arrayidx.i, align 4
+  %vecinit3.i.i.1 = insertelement <4 x i32> <i32 undef, i32 0, i32 undef, i32 undef>, i32 %5, i32 0
+  %6 = bitcast <4 x i32> %vecinit3.i.i.1 to <2 x i64>
+  %7 = load i32, i32* %arrayidx2.i, align 4
+  %vecinit3.i10.i.1 = insertelement <4 x i32> <i32 undef, i32 0, i32 undef, i32 undef>, i32 %7, i32 0
+  %8 = bitcast <4 x i32> %vecinit3.i10.i.1 to <2 x i64>
+  %shuffle.i.i.1 = shufflevector <2 x i64> %6, <2 x i64> %8, <2 x i32> <i32 0, i32 2>
+  %add.i.i.1 = add <2 x i64> %shuffle.i.i.1, %add.i.i
+  store <2 x i64> %add.i.i.1, <2 x i64>* %res, align 16
+  %9 = load i32, i32* %arrayidx.i, align 4
+  %vecinit3.i.i.2 = insertelement <4 x i32> <i32 undef, i32 0, i32 undef, i32 undef>, i32 %9, i32 0
+  %10 = bitcast <4 x i32> %vecinit3.i.i.2 to <2 x i64>
+  %11 = load i32, i32* %arrayidx2.i, align 4
+  %vecinit3.i10.i.2 = insertelement <4 x i32> <i32 undef, i32 0, i32 undef, i32 undef>, i32 %11, i32 0
+  %12 = bitcast <4 x i32> %vecinit3.i10.i.2 to <2 x i64>
+  %shuffle.i.i.2 = shufflevector <2 x i64> %10, <2 x i64> %12, <2 x i32> <i32 0, i32 2>
+  %add.i.i.2 = add <2 x i64> %shuffle.i.i.2, %add.i.i.1
+  store <2 x i64> %add.i.i.2, <2 x i64>* %res, align 16
+  %13 = load i32, i32* %arrayidx.i, align 4
+  %vecinit3.i.i.3 = insertelement <4 x i32> <i32 undef, i32 0, i32 undef, i32 undef>, i32 %13, i32 0
+  %14 = bitcast <4 x i32> %vecinit3.i.i.3 to <2 x i64>
+  %15 = load i32, i32* %arrayidx2.i, align 4
+  %vecinit3.i10.i.3 = insertelement <4 x i32> <i32 undef, i32 0, i32 undef, i32 undef>, i32 %15, i32 0
+  %16 = bitcast <4 x i32> %vecinit3.i10.i.3 to <2 x i64>
+  %shuffle.i.i.3 = shufflevector <2 x i64> %14, <2 x i64> %16, <2 x i32> <i32 0, i32 2>
+  %add.i.i.3 = add <2 x i64> %shuffle.i.i.3, %add.i.i.2
+  store <2 x i64> %add.i.i.3, <2 x i64>* %res, align 16
+  ret void
+}