[X86] In decomposeMulByConstant, legalize the VT before querying whether the multiply is legal

[craig.topper]

Summary

If a type is larger than a legal type and needs to be split, we would previously allow the multiply to be decomposed even if the split multiply is legal. Since the shift + add/sub code would also need to be split, its not any better to decompose it.

This patch figures out what type the mul will eventually be legalized to and then uses that type for the query. I tried just returning false illegal types and letting them get handled after type legalization, but then we can't recognize and i64 constant splat on 32-bit targets since will be destroyed by type legalization. We could special case vectors of i64 to avoid that...

[spatel]

LGTM

llvm/lib/Target/X86/X86ISelLowering.cpp

4891–4892 ↗

(On Diff #212646)

This comment should include the reasoning from the patch description. It won't be obvious when scanning the code in the future why we chose this logic over a simpler legality check.

4893 ↗

(On Diff #212646)

Don't need the temp variable if the original VT is never used again?

llvm/trunk/include/llvm/CodeGen/TargetLowering.h

virtual bool convertSelectOfConstantsToMath(EVT VT) const {
     return false;
   }
 
   /// Return true if it is profitable to transform an integer
   /// multiplication-by-constant into simpler operations like shifts and adds.
   /// This may be true if the target does not directly support the
   /// multiplication operation for the specified type or the sequence of simpler
   /// ops is faster than the multiply.
-  virtual bool decomposeMulByConstant(EVT VT, SDValue C) const {
+  virtual bool decomposeMulByConstant(LLVMContext &Context,
+                                      EVT VT, SDValue C) const {
     return false;
   }
 
   /// Return true if it is more correct/profitable to use strict FP_TO_INT
   /// conversion operations - canonicalizing the FP source value instead of
   /// converting all cases and then selecting based on value.
   /// This may be true if the target throws exceptions for out of bounds
   /// conversions or has fast FP CMOV.

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitMUL(SDNode *N) {
 
   // Try to transform multiply-by-(power-of-2 +/- 1) into shift and add/sub.
   // mul x, (2^N + 1) --> add (shl x, N), x
   // mul x, (2^N - 1) --> sub (shl x, N), x
   // Examples: x * 33 --> (x << 5) + x
   //           x * 15 --> (x << 4) - x
   //           x * -33 --> -((x << 5) + x)
   //           x * -15 --> -((x << 4) - x) ; this reduces --> x - (x << 4)
-  if (N1IsConst && TLI.decomposeMulByConstant(VT, N1)) {
+  if (N1IsConst && TLI.decomposeMulByConstant(*DAG.getContext(), VT, N1)) {
     // TODO: We could handle more general decomposition of any constant by
     //       having the target set a limit on number of ops and making a
     //       callback to determine that sequence (similar to sqrt expansion).
     unsigned MathOp = ISD::DELETED_NODE;
     APInt MulC = ConstValue1.abs();
     if ((MulC - 1).isPowerOf2())
       MathOp = ISD::ADD;
     else if ((MulC + 1).isPowerOf2())

llvm/trunk/lib/Target/X86/X86ISelLowering.h

public:
     /// to just the constant itself.
     bool shouldConvertConstantLoadToIntImm(const APInt &Imm,
                                            Type *Ty) const override;
 
     bool reduceSelectOfFPConstantLoads(bool IsFPSetCC) const override;
 
     bool convertSelectOfConstantsToMath(EVT VT) const override;
 
-    bool decomposeMulByConstant(EVT VT, SDValue C) const override;
+    bool decomposeMulByConstant(LLVMContext &Context, EVT VT,
+                                SDValue C) const override;
 
     bool shouldUseStrictFP_TO_INT(EVT FpVT, EVT IntVT,
                                   bool IsSigned) const override;
 
     /// Return true if EXTRACT_SUBVECTOR is cheap for this result type
     /// with this index.
     bool isExtractSubvectorCheap(EVT ResVT, EVT SrcVT,
                                  unsigned Index) const override;

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

bool X86TargetLowering::convertSelectOfConstantsToMath(EVT VT) const {
   // TODO: It might be a win to ease or lift this restriction, but the generic
   // folds in DAGCombiner conflict with vector folds for an AVX512 target.
   if (VT.isVector() && Subtarget.hasAVX512())
     return false;
 
   return true;
 }
 
-bool X86TargetLowering::decomposeMulByConstant(EVT VT, SDValue C) const {
+bool X86TargetLowering::decomposeMulByConstant(LLVMContext &Context, EVT VT,
+                                               SDValue C) const {
   // TODO: We handle scalars using custom code, but generic combining could make
   // that unnecessary.
   APInt MulC;
   if (!ISD::isConstantSplatVector(C.getNode(), MulC))
     return false;
 
+  // Find the type this will be legalized too. Otherwise we might prematurely
+  // convert this to shl+add/sub and then still have to type legalize those ops.
+  // Another choice would be to defer the decision for illegal types until 
+  // after type legalization. But constant splat vectors of i64 can't make it
+  // through type legalization on 32-bit targets so we would need to special
+  // case vXi64.
+  while (getTypeAction(Context, VT) != TypeLegal)
+    VT = getTypeToTransformTo(Context, VT);
+
   // If vector multiply is legal, assume that's faster than shl + add/sub.
-  // TODO: Multiply is a complex op with higher latency and lower througput in
+  // TODO: Multiply is a complex op with higher latency and lower throughput in
   //       most implementations, so this check could be loosened based on type
   //       and/or a CPU attribute.
   if (isOperationLegal(ISD::MUL, VT))
     return false;
 
   // shl+add, shl+sub, shl+add+neg
   return (MulC + 1).isPowerOf2() || (MulC - 1).isPowerOf2() ||
          (1 - MulC).isPowerOf2() || (-(MulC + 1)).isPowerOf2();

llvm/trunk/test/CodeGen/X86/vector-mul.ll

 ; X64-AVX512DQ-NEXT:    retq
   %1 = mul <4 x i64> %a0, <i64 17, i64 17, i64 17, i64 17>
   ret <4 x i64> %1
 }
 
 define <8 x i32> @mul_v8i32_17(<8 x i32> %a0) nounwind {
 ; X86-LABEL: mul_v8i32_17:
 ; X86:       # %bb.0:
-; X86-NEXT:    movdqa %xmm0, %xmm2
-; X86-NEXT:    pslld $4, %xmm2
-; X86-NEXT:    paddd %xmm0, %xmm2
-; X86-NEXT:    movdqa %xmm1, %xmm3
-; X86-NEXT:    pslld $4, %xmm3
-; X86-NEXT:    paddd %xmm1, %xmm3
-; X86-NEXT:    movdqa %xmm2, %xmm0
-; X86-NEXT:    movdqa %xmm3, %xmm1
+; X86-NEXT:    movdqa {{.*#+}} xmm2 = [17,17,17,17]
+; X86-NEXT:    pmulld %xmm2, %xmm0
+; X86-NEXT:    pmulld %xmm2, %xmm1
 ; X86-NEXT:    retl
 ;
 ; X64-LABEL: mul_v8i32_17:
 ; X64:       # %bb.0:
-; X64-NEXT:    movdqa %xmm0, %xmm2
-; X64-NEXT:    pslld $4, %xmm2
-; X64-NEXT:    paddd %xmm0, %xmm2
-; X64-NEXT:    movdqa %xmm1, %xmm3
-; X64-NEXT:    pslld $4, %xmm3
-; X64-NEXT:    paddd %xmm1, %xmm3
-; X64-NEXT:    movdqa %xmm2, %xmm0
-; X64-NEXT:    movdqa %xmm3, %xmm1
+; X64-NEXT:    movdqa {{.*#+}} xmm2 = [17,17,17,17]
+; X64-NEXT:    pmulld %xmm2, %xmm0
+; X64-NEXT:    pmulld %xmm2, %xmm1
 ; X64-NEXT:    retq
 ;
 ; X64-XOP-LABEL: mul_v8i32_17:
 ; X64-XOP:       # %bb.0:
 ; X64-XOP-NEXT:    vextractf128 $1, %ymm0, %xmm1
 ; X64-XOP-NEXT:    vpslld $4, %xmm1, %xmm2
 ; X64-XOP-NEXT:    vpaddd %xmm1, %xmm2, %xmm1
 ; X64-XOP-NEXT:    vpslld $4, %xmm0, %xmm2

 ; X64-AVX512DQ-NEXT:    retq
   %1 = mul <8 x i32> %a0, <i32 17, i32 17, i32 17, i32 17, i32 17, i32 17, i32 17, i32 17>
   ret <8 x i32> %1
 }
 
 define <16 x i16> @mul_v16i16_17(<16 x i16> %a0) nounwind {
 ; X86-LABEL: mul_v16i16_17:
 ; X86:       # %bb.0:
-; X86-NEXT:    movdqa %xmm0, %xmm2
-; X86-NEXT:    psllw $4, %xmm2
-; X86-NEXT:    paddw %xmm0, %xmm2
-; X86-NEXT:    movdqa %xmm1, %xmm3
-; X86-NEXT:    psllw $4, %xmm3
-; X86-NEXT:    paddw %xmm1, %xmm3
-; X86-NEXT:    movdqa %xmm2, %xmm0
-; X86-NEXT:    movdqa %xmm3, %xmm1
+; X86-NEXT:    movdqa {{.*#+}} xmm2 = [17,17,17,17,17,17,17,17]
+; X86-NEXT:    pmullw %xmm2, %xmm0
+; X86-NEXT:    pmullw %xmm2, %xmm1
 ; X86-NEXT:    retl
 ;
 ; X64-LABEL: mul_v16i16_17:
 ; X64:       # %bb.0:
-; X64-NEXT:    movdqa %xmm0, %xmm2
-; X64-NEXT:    psllw $4, %xmm2
-; X64-NEXT:    paddw %xmm0, %xmm2
-; X64-NEXT:    movdqa %xmm1, %xmm3
-; X64-NEXT:    psllw $4, %xmm3
-; X64-NEXT:    paddw %xmm1, %xmm3
-; X64-NEXT:    movdqa %xmm2, %xmm0
-; X64-NEXT:    movdqa %xmm3, %xmm1
+; X64-NEXT:    movdqa {{.*#+}} xmm2 = [17,17,17,17,17,17,17,17]
+; X64-NEXT:    pmullw %xmm2, %xmm0
+; X64-NEXT:    pmullw %xmm2, %xmm1
 ; X64-NEXT:    retq
 ;
 ; X64-XOP-LABEL: mul_v16i16_17:
 ; X64-XOP:       # %bb.0:
 ; X64-XOP-NEXT:    vextractf128 $1, %ymm0, %xmm1
 ; X64-XOP-NEXT:    vpsllw $4, %xmm1, %xmm2
 ; X64-XOP-NEXT:    vpaddw %xmm1, %xmm2, %xmm1
 ; X64-XOP-NEXT:    vpsllw $4, %xmm0, %xmm2

 ; X64-AVX512DQ-NEXT:    retq
   %1 = mul <4 x i64> %a0, <i64 -1025, i64 -1025, i64 -1025, i64 -1025>
   ret <4 x i64> %1
 }
 
 define <8 x i32> @mul_v8i32_neg33(<8 x i32> %a0) nounwind {
 ; X86-LABEL: mul_v8i32_neg33:
 ; X86:       # %bb.0:
-; X86-NEXT:    movdqa %xmm0, %xmm3
-; X86-NEXT:    pslld $5, %xmm3
-; X86-NEXT:    paddd %xmm0, %xmm3
-; X86-NEXT:    pxor %xmm2, %xmm2
-; X86-NEXT:    pxor %xmm0, %xmm0
-; X86-NEXT:    psubd %xmm3, %xmm0
-; X86-NEXT:    movdqa %xmm1, %xmm3
-; X86-NEXT:    pslld $5, %xmm3
-; X86-NEXT:    paddd %xmm1, %xmm3
-; X86-NEXT:    psubd %xmm3, %xmm2
-; X86-NEXT:    movdqa %xmm2, %xmm1
+; X86-NEXT:    movdqa {{.*#+}} xmm2 = [4294967263,4294967263,4294967263,4294967263]
+; X86-NEXT:    pmulld %xmm2, %xmm0
+; X86-NEXT:    pmulld %xmm2, %xmm1
 ; X86-NEXT:    retl
 ;
 ; X64-LABEL: mul_v8i32_neg33:
 ; X64:       # %bb.0:
-; X64-NEXT:    movdqa %xmm0, %xmm3
-; X64-NEXT:    pslld $5, %xmm3
-; X64-NEXT:    paddd %xmm0, %xmm3
-; X64-NEXT:    pxor %xmm2, %xmm2
-; X64-NEXT:    pxor %xmm0, %xmm0
-; X64-NEXT:    psubd %xmm3, %xmm0
-; X64-NEXT:    movdqa %xmm1, %xmm3
-; X64-NEXT:    pslld $5, %xmm3
-; X64-NEXT:    paddd %xmm1, %xmm3
-; X64-NEXT:    psubd %xmm3, %xmm2
-; X64-NEXT:    movdqa %xmm2, %xmm1
+; X64-NEXT:    movdqa {{.*#+}} xmm2 = [4294967263,4294967263,4294967263,4294967263]
+; X64-NEXT:    pmulld %xmm2, %xmm0
+; X64-NEXT:    pmulld %xmm2, %xmm1
 ; X64-NEXT:    retq
 ;
 ; X64-XOP-LABEL: mul_v8i32_neg33:
 ; X64-XOP:       # %bb.0:
 ; X64-XOP-NEXT:    vextractf128 $1, %ymm0, %xmm1
 ; X64-XOP-NEXT:    vpslld $5, %xmm1, %xmm2
 ; X64-XOP-NEXT:    vpaddd %xmm1, %xmm2, %xmm1
 ; X64-XOP-NEXT:    vpxor %xmm2, %xmm2, %xmm2

 ; X64-AVX512DQ-NEXT:    retq
   %1 = mul <8 x i32> %a0, <i32 -33, i32 -33, i32 -33, i32 -33, i32 -33, i32 -33, i32 -33, i32 -33>
   ret <8 x i32> %1
 }
 
 define <16 x i16> @mul_v16i16_neg9(<16 x i16> %a0) nounwind {
 ; X86-LABEL: mul_v16i16_neg9:
 ; X86:       # %bb.0:
-; X86-NEXT:    movdqa %xmm0, %xmm3
-; X86-NEXT:    psllw $3, %xmm3
-; X86-NEXT:    paddw %xmm0, %xmm3
-; X86-NEXT:    pxor %xmm2, %xmm2
-; X86-NEXT:    pxor %xmm0, %xmm0
-; X86-NEXT:    psubw %xmm3, %xmm0
-; X86-NEXT:    movdqa %xmm1, %xmm3
-; X86-NEXT:    psllw $3, %xmm3
-; X86-NEXT:    paddw %xmm1, %xmm3
-; X86-NEXT:    psubw %xmm3, %xmm2
-; X86-NEXT:    movdqa %xmm2, %xmm1
+; X86-NEXT:    movdqa {{.*#+}} xmm2 = [65527,65527,65527,65527,65527,65527,65527,65527]
+; X86-NEXT:    pmullw %xmm2, %xmm0
+; X86-NEXT:    pmullw %xmm2, %xmm1
 ; X86-NEXT:    retl
 ;
 ; X64-LABEL: mul_v16i16_neg9:
 ; X64:       # %bb.0:
-; X64-NEXT:    movdqa %xmm0, %xmm3
-; X64-NEXT:    psllw $3, %xmm3
-; X64-NEXT:    paddw %xmm0, %xmm3
-; X64-NEXT:    pxor %xmm2, %xmm2
-; X64-NEXT:    pxor %xmm0, %xmm0
-; X64-NEXT:    psubw %xmm3, %xmm0
-; X64-NEXT:    movdqa %xmm1, %xmm3
-; X64-NEXT:    psllw $3, %xmm3
-; X64-NEXT:    paddw %xmm1, %xmm3
-; X64-NEXT:    psubw %xmm3, %xmm2
-; X64-NEXT:    movdqa %xmm2, %xmm1
+; X64-NEXT:    movdqa {{.*#+}} xmm2 = [65527,65527,65527,65527,65527,65527,65527,65527]
+; X64-NEXT:    pmullw %xmm2, %xmm0
+; X64-NEXT:    pmullw %xmm2, %xmm1
 ; X64-NEXT:    retq
 ;
 ; X64-XOP-LABEL: mul_v16i16_neg9:
 ; X64-XOP:       # %bb.0:
 ; X64-XOP-NEXT:    vextractf128 $1, %ymm0, %xmm1
 ; X64-XOP-NEXT:    vpsllw $3, %xmm1, %xmm2
 ; X64-XOP-NEXT:    vpaddw %xmm1, %xmm2, %xmm1
 ; X64-XOP-NEXT:    vpxor %xmm2, %xmm2, %xmm2