[X86] Fix bug in r288804.

[aymanmus]

Summary

When creating the load from constant pool, check if there was already a load to the same constant, and abort if found.
The additional use of the load will prevent the pattern from memory folding the broadcast.

[zvi]

Please add a test

[aymanmus]

+Test

[mkuper]

A drive-by comment - regardless of whether we should be forming a vbroadcast for this case (I'll let you and Zvi figure it out :-) ), shouldn't we also have a pattern in place for the case we do?

X86InstrAVX512.td has a lot of X86SubVBroadcast patterns that are commented with:
Provide fallback in case the load node that is used in the patterns above
is used by additional users, which prevents the pattern selection.
But I don't see such a pattern for the v4i64 -> v8i64 case.

[andreadb]

lib/Target/X86/X86ISelLowering.cpp

6414–6415

X86ISD::VBROADCAST requires a memory operand only if the target is not AVX2.
On AVX2, we should still be able to select a register-register variant of VBROADCAST from a X86ISD::VBROADCAST.
Basically (at least on AVX2) I don't think that a X86ISD::VBROADCAST is problematic even if we have load with multiple uses.

6435–6436

Would it be possible to check in advance if the Ld node is already used?
In case, we would know in advance that the check at line 6455 would fail. That would let us early exit before we even create a node for the broadcast.

6445–6449

Same as above. Early exit if we know in advance that Ld has already uses.

6450–6455

I think it is safe to remove the else part.
You can always check if Brdcst is null at line 6455.

I think you can write something like this:

if (Brdcst && !Ld.hasOneUse())

6453–6456

Phabricator doesn't let me delete this comment...

[zvi]

Ayman, I think that @mkuper 's observation is the way to fix the PR you are addressing, followed by patches for the optimization in this patch, and @andreadb's suggestions.

[aymanmus]

A new review with the solution - offered by @mkuper - is up for review:
https://reviews.llvm.org/D27661

lib/Target/X86/X86ISelLowering.cpp

if (BVOp->isConstantSplat(SplatValue, Undef, SplatBitSize, HasUndef) &&
       // Avoid replacing with broadcast when it's a use of a shuffle
       // instruction to preserve the present custom lowering of shuffles.
       if (isUseOfShuffle(BVOp) || BVOp->hasOneUse())
         return SDValue();
       // replace BUILD_VECTOR with broadcast of the repeated constants.
       const TargetLowering &TLI = DAG.getTargetLoweringInfo();
       LLVMContext *Ctx = DAG.getContext();
       MVT PVT = TLI.getPointerTy(DAG.getDataLayout());
+      SDValue Brdcst;
       if (Subtarget.hasAVX()) {
         if (SplatBitSize <= 64 && Subtarget.hasAVX2() &&
             !(SplatBitSize == 64 && Subtarget.is32Bit())) {
           // Splatted value can fit in one INTEGER constant in constant pool.
           // Load the constant and broadcast it.
           MVT CVT = MVT::getIntegerVT(SplatBitSize);
           Type *ScalarTy = Type::getIntNTy(*Ctx, SplatBitSize);
           Constant *C = Constant::getIntegerValue(ScalarTy, SplatValue);
           SDValue CP = DAG.getConstantPool(C, PVT);
           unsigned Repeat = VT.getSizeInBits() / SplatBitSize;
 
           unsigned Alignment = cast<ConstantPoolSDNode>(CP)->getAlignment();
           Ld = DAG.getLoad(
               CVT, dl, DAG.getEntryNode(), CP,
               MachinePointerInfo::getConstantPool(DAG.getMachineFunction()),
               Alignment);
-          SDValue Brdcst = DAG.getNode(X86ISD::VBROADCAST, dl,
+          Brdcst = DAG.getNode(X86ISD::VBROADCAST, dl,
                                        MVT::getVectorVT(CVT, Repeat), Ld);
-          return DAG.getBitcast(VT, Brdcst);
         } else if (SplatBitSize == 32 || SplatBitSize == 64) {
           // Splatted value can fit in one FLOAT constant in constant pool.
           // Load the constant and broadcast it.
           // AVX have support for 32 and 64 bit broadcast for floats only.
           // No 64bit integer in 32bit subtarget.
           MVT CVT = MVT::getFloatingPointVT(SplatBitSize);
           Constant *C = SplatBitSize == 32
                             ? ConstantFP::get(Type::getFloatTy(*Ctx),
                                               SplatValue.bitsToFloat())
                             : ConstantFP::get(Type::getDoubleTy(*Ctx),
                                               SplatValue.bitsToDouble());
           SDValue CP = DAG.getConstantPool(C, PVT);
           unsigned Repeat = VT.getSizeInBits() / SplatBitSize;
 
           unsigned Alignment = cast<ConstantPoolSDNode>(CP)->getAlignment();
           Ld = DAG.getLoad(
               CVT, dl, DAG.getEntryNode(), CP,
               MachinePointerInfo::getConstantPool(DAG.getMachineFunction()),
               Alignment);
-          SDValue Brdcst = DAG.getNode(X86ISD::VBROADCAST, dl,
+          Brdcst = DAG.getNode(X86ISD::VBROADCAST, dl,
                                        MVT::getVectorVT(CVT, Repeat), Ld);
-          return DAG.getBitcast(VT, Brdcst);
         } else if (SplatBitSize > 64) {
           // Load the vector of constants and broadcast it.
           MVT CVT = VT.getScalarType();
           Constant *VecC = getConstantVector(VT, SplatValue, SplatBitSize,
                                              *Ctx);
           SDValue VCP = DAG.getConstantPool(VecC, PVT);
           unsigned NumElm = SplatBitSize / VT.getScalarSizeInBits();
           unsigned Alignment = cast<ConstantPoolSDNode>(VCP)->getAlignment();
           Ld = DAG.getLoad(
               MVT::getVectorVT(CVT, NumElm), dl, DAG.getEntryNode(), VCP,
               MachinePointerInfo::getConstantPool(DAG.getMachineFunction()),
               Alignment);
-          SDValue Brdcst = DAG.getNode(X86ISD::SUBV_BROADCAST, dl, VT, Ld);
-          return DAG.getBitcast(VT, Brdcst);
+          Brdcst = DAG.getNode(X86ISD::SUBV_BROADCAST, dl, VT, Ld);
+        } else {
+          return SDValue();
         }
+        // If the load already had a use, we can't fold it into the broadcast.
+        // So fallback to the original state.
+        if (!Ld.hasOneUse())
+          return SDValue();
+        return DAG.getBitcast(VT, Brdcst);
       }
     }
     return SDValue();
   }
 
   bool ConstSplatVal =
       (Ld.getOpcode() == ISD::Constant || Ld.getOpcode() == ISD::ConstantFP);
 

test/CodeGen/X86/pr31306.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc %s -O2 -mattr=avx512f | FileCheck %s
+
+%struct.i = type { %struct.a, %struct.a }
+%struct.a = type { i48, i40, i48, i40 }
+
+@m = local_unnamed_addr global %struct.i zeroinitializer, align 8
+@n = local_unnamed_addr global %struct.i zeroinitializer, align 8
+@_ZN1i1lE = external local_unnamed_addr global %struct.a, align 8
+
+; Function Attrs: norecurse nounwind uwtable
+define void @_Z1ov() local_unnamed_addr {
+; CHECK-LABEL: _Z1ov:
+; CHECK:       # BB#0: # %entry
+; CHECK-NEXT:    vmovups {{.*}}(%rip), %ymm0
+; CHECK-NEXT:    vandps {{.*}}(%rip), %ymm0, %ymm0
+; CHECK-NEXT:    vorps {{.*}}(%rip), %ymm0, %ymm0
+; CHECK-NEXT:    vmovups %ymm0, {{.*}}(%rip)
+; CHECK-NEXT:    vmovdqa64 {{.*#+}} zmm0 = [18446744073709547520,18446742974197923840,18446744069414584320,18446744065119617024,18446744073709547520,18446742974197923840,18446744069414584320,18446744065119617024]
+; CHECK-NEXT:    vpandq {{.*}}(%rip), %zmm0, %zmm1
+; CHECK-NEXT:    vbroadcasti64x4 {{.*#+}} zmm2 = mem[0,1,2,3,0,1,2,3]
+; CHECK-NEXT:    vporq %zmm2, %zmm1, %zmm1
+; CHECK-NEXT:    vmovdqu64 %zmm1, {{.*}}(%rip)
+; CHECK-NEXT:    vpandq {{.*}}(%rip), %zmm0, %zmm0
+; CHECK-NEXT:    vporq %zmm2, %zmm0, %zmm0
+; CHECK-NEXT:    vmovdqu64 %zmm0, {{.*}}(%rip)
+; CHECK-NEXT:    retq
+entry:
+  %0 = load <4 x i64>, <4 x i64>* bitcast (%struct.a* @_ZN1i1lE to <4 x i64>*), align 8
+  %1 = and <4 x i64> %0, <i64 -4096, i64 -1099511627776, i64 -4294967296, i64 -8589934592>
+  %2 = or <4 x i64> %1, <i64 4094, i64 961535803390, i64 6, i64 100663302>
+  store <4 x i64> %2, <4 x i64>* bitcast (%struct.a* @_ZN1i1lE to <4 x i64>*), align 8
+  %3 = load <8 x i64>, <8 x i64>* bitcast (%struct.i* @n to <8 x i64>*), align 8
+  %4 = and <8 x i64> %3, <i64 -4096, i64 -1099511627776, i64 -4294967296, i64 -8589934592, i64 -4096, i64 -1099511627776, i64 -4294967296, i64 -8589934592>
+  %5 = or <8 x i64> %4, <i64 4094, i64 961535803390, i64 4294967294, i64 8573157374, i64 4094, i64 961535803390, i64 4294967294, i64 8573157374>
+  store <8 x i64> %5, <8 x i64>* bitcast (%struct.i* @n to <8 x i64>*), align 8
+  %6 = load <8 x i64>, <8 x i64>* bitcast (%struct.i* @m to <8 x i64>*), align 8
+  %7 = and <8 x i64> %6, <i64 -4096, i64 -1099511627776, i64 -4294967296, i64 -8589934592, i64 -4096, i64 -1099511627776, i64 -4294967296, i64 -8589934592>
+  %8 = or <8 x i64> %7, <i64 4094, i64 961535803390, i64 4294967294, i64 8573157374, i64 4094, i64 961535803390, i64 4294967294, i64 8573157374>
+  store <8 x i64> %8, <8 x i64>* bitcast (%struct.i* @m to <8 x i64>*), align 8
+  ret void
+}
+