AVX512F: FMA intrinsic + FNEG - sequence optimization

[delena]

Summary

The previous patch https://reviews.llvm.org/D23313 does not cover AVX-512F, KNL set.
FNEG(x) operation is lowered to (bitcast (vpxor (bitcast x), (bitcast constfp(0x80000000))).
It happens because FP XOR is not supported for 512-bit data types on KNL and we use integer XOR instead.
I added pattern match for integer XOR.

[spatel]

../lib/Target/X86/X86ISelLowering.cpp

30304

has -> have

30359–30361

Can this be an assert?
assert(IsFNEGNode && "Expected FNEG node");

30384–30393

This would be clearer/smaller if we switched just the opcode selection and sank the common 'getBitcast(getNode())' after the switch:

auto NewOpcode;
switch (Arg.getOpcode()) {
case X86ISD::FMADD: NewOpcode = X86ISD::FNMSUB; break;
case X86ISD::FMSUB: NewOpcode = X86ISD::FNMADD; break;
...
}
return DAG.getBitcast(OrigVT, DAG.getNode(NewOpcode, DL, VT, Arg.getNode()->ops());

If you do that refactoring on the original code as an NFC patch, I think it will make the diff on this part of this patch just one line of code.

[delena]

Some code style improvements after Sanjay's comments.

[spatel]

../lib/Target/X86/X86ISelLowering.cpp

30307

This interface feels awkward. The normal pattern is:
static SDValue getFNeg(SDNode *N)

Ie, if there's no negated value, it will return the empty SDValue(), and the caller can use that as a bool value if it wants to.

30359–30361

This question was missed?

[delena]

../lib/Target/X86/X86ISelLowering.cpp

30307

Thanks! I was looking for a proper interface. You suggestion is better than all APIs that I tried till now.
I'll update the patch.

I still think that my first solution with late FNEG lowering was simpler. We handle too many different patterns for XOR/FXOR.

30359–30361

It will fail on compilation in release mode:
bool IsFNEGNode = isFNEG(N, &Arg); - Unused variable IsFNEGNode
assert(IsFNEGNode && "Expected FNEG node");

[delena]

Changed isFNEG() interface.

[spatel]

LGTM - see inline comments for some nits.

../lib/Target/X86/X86ISelLowering.cpp

30303

Fp -> FP

30307

static?

30877

formatting: SDValue &V

30878–30879

combine to one line? if (SDValue Negval = isFNEG...)

../lib/Target/X86/X86ISelLowering.cpp

static SDValue foldVectorXorShiftIntoCmp(SDNode *N, SelectionDAG &DAG,
   if (!ShiftAmt || ShiftAmt->getZExtValue() != ShiftEltTy.getSizeInBits() - 1)
     return SDValue();
 
   // Create a greater-than comparison against -1. We don't use the more obvious
   // greater-than-or-equal-to-zero because SSE/AVX don't have that instruction.
   return DAG.getNode(X86ISD::PCMPGT, SDLoc(N), VT, Shift.getOperand(0), Ones);
 }
 
-static SDValue combineXor(SDNode *N, SelectionDAG &DAG,
-                                 TargetLowering::DAGCombinerInfo &DCI,
-                                 const X86Subtarget &Subtarget) {
-  if (SDValue Cmp = foldVectorXorShiftIntoCmp(N, DAG, Subtarget))
-    return Cmp;
-
-  if (DCI.isBeforeLegalizeOps())
-    return SDValue();
-
-  if (SDValue RV = foldXorTruncShiftIntoCmp(N, DAG))
-    return RV;
-
-  if (Subtarget.hasCMov())
-    if (SDValue RV = combineIntegerAbs(N, DAG))
-      return RV;
-
-  if (SDValue FPLogic = convertIntLogicToFPLogic(N, DAG, Subtarget))
-    return FPLogic;
-
-  return SDValue();
-}
-
 /// This function detects the AVG pattern between vectors of unsigned i8/i16,
 /// which is c = (a + b + 1) / 2, and replace this operation with the efficient
 /// X86ISD::AVG instruction.
 static SDValue detectAVGPattern(SDValue In, EVT VT, SelectionDAG &DAG,
                                 const X86Subtarget &Subtarget,
                                 const SDLoc &DL) {
   if (!VT.isVector() || !VT.isSimple())
     return SDValue();
static SDValue combineTruncate(SDNode *N, SelectionDAG &DAG,
 
   // Try to truncate vector comparison results with PACKSS.
   if (SDValue V = combineVectorCompareTruncation(N, DL, DAG, Subtarget))
     return V;
 
   return combineVectorTruncation(N, DAG, Subtarget);
 }
 
+/// Returns the negated value if the node \p N flips sign of FP value.
+///
+/// Fp-negation node may have different forms: FNEG(x) or FXOR (x, 0x80000000).
+/// AVX512F does not have FXOR, so FNEG is lowered as
+/// (bitcast (xor (bitcast x), (bitcast ConstantFP(0x80000000)))).
+/// In this case we go though all bitcasts.
+SDValue isFNEG(SDNode *N) {
+  if (N->getOpcode() == ISD::FNEG)
+    return N->getOperand(0);
+
+  SDValue Op = peekThroughBitcasts(SDValue(N, 0));
+  if (Op.getOpcode() != X86ISD::FXOR && Op.getOpcode() != ISD::XOR)
+    return SDValue();
+
+  SDValue Op1 = peekThroughBitcasts(Op.getOperand(1));
+  if (!Op1.getValueType().isFloatingPoint())
+    return SDValue();
+
+  SDValue Op0 = peekThroughBitcasts(Op.getOperand(0));
+
+  unsigned EltBits = Op1.getValueType().getScalarSizeInBits();
+  auto isSignBitValue = [&](const ConstantFP *C) {
+    return C->getValueAPF().bitcastToAPInt() == APInt::getSignBit(EltBits);
+  };
+
+  // There is more than one way to represent the same constant on
+  // the different X86 targets. The type of the node may also depend on size.
+  //  - load scalar value and broadcast
+  //  - BUILD_VECTOR node
+  //  - load from a constant pool.
+  // We check all variants here.
+  if (Op1.getOpcode() == X86ISD::VBROADCAST) {
+    if (auto *C = getTargetConstantFromNode(Op1.getOperand(0)))
+      if (isSignBitValue(cast<ConstantFP>(C)))
+        return Op0;
+
+  } else if (BuildVectorSDNode *BV = dyn_cast<BuildVectorSDNode>(Op1)) {
+    if (ConstantFPSDNode *CN = BV->getConstantFPSplatNode())
+      if (isSignBitValue(CN->getConstantFPValue()))
+        return Op0;
+
+  } else if (auto *C = getTargetConstantFromNode(Op1)) {
+    if (C->getType()->isVectorTy()) {
+      if (auto *SplatV = C->getSplatValue())
+        if (isSignBitValue(cast<ConstantFP>(SplatV)))
+          return Op0;
+    } else if (auto *FPConst = dyn_cast<ConstantFP>(C))
+      if (isSignBitValue(FPConst))
+        return Op0;
+  }
+  return SDValue();
+}
+
 /// Do target-specific dag combines on floating point negations.
 static SDValue combineFneg(SDNode *N, SelectionDAG &DAG,
                            const X86Subtarget &Subtarget) {
-  EVT VT = N->getValueType(0);
+  EVT OrigVT = N->getValueType(0);
+  SDValue Arg = isFNEG(N);
+  assert(Arg.getNode() && "N is expected to be an FNEG node");
+
+  EVT VT = Arg.getValueType();
   EVT SVT = VT.getScalarType();
-  SDValue Arg = N->getOperand(0);
   SDLoc DL(N);
 
   // Let legalize expand this if it isn't a legal type yet.
   if (!DAG.getTargetLoweringInfo().isTypeLegal(VT))
     return SDValue();
 
   // If we're negating a FMUL node on a target with FMA, then we can avoid the
   // use of a constant by performing (-0 - A*B) instead.
   // FIXME: Check rounding control flags as well once it becomes available.
   if (Arg.getOpcode() == ISD::FMUL && (SVT == MVT::f32 || SVT == MVT::f64) &&
       Arg->getFlags()->hasNoSignedZeros() && Subtarget.hasAnyFMA()) {
     SDValue Zero = DAG.getConstantFP(0.0, DL, VT);
-    return DAG.getNode(X86ISD::FNMSUB, DL, VT, Arg.getOperand(0),
+    SDValue NewNode = DAG.getNode(X86ISD::FNMSUB, DL, VT, Arg.getOperand(0),
                                   Arg.getOperand(1), Zero);
+    return DAG.getBitcast(OrigVT, NewNode);
   }
 
   // If we're negating a FMA node, then we can adjust the
   // instruction to include the extra negation.
+  unsigned NewOpcode = 0;
   if (Arg.hasOneUse()) {
     switch (Arg.getOpcode()) {
-    case X86ISD::FMADD:
-      return DAG.getNode(X86ISD::FNMSUB, DL, VT, Arg.getOperand(0),
-                         Arg.getOperand(1), Arg.getOperand(2));
-    case X86ISD::FMSUB:
-      return DAG.getNode(X86ISD::FNMADD, DL, VT, Arg.getOperand(0),
-                         Arg.getOperand(1), Arg.getOperand(2));
-    case X86ISD::FNMADD:
-      return DAG.getNode(X86ISD::FMSUB, DL, VT, Arg.getOperand(0),
-                         Arg.getOperand(1), Arg.getOperand(2));
-    case X86ISD::FNMSUB:
-      return DAG.getNode(X86ISD::FMADD, DL, VT, Arg.getOperand(0),
-                         Arg.getOperand(1), Arg.getOperand(2));
-    case X86ISD::FMADD_RND:
-      return DAG.getNode(X86ISD::FNMSUB_RND, DL, VT, Arg.getOperand(0),
-                         Arg.getOperand(1), Arg.getOperand(2), Arg.getOperand(3));
-    case X86ISD::FMSUB_RND:
-      return DAG.getNode(X86ISD::FNMADD_RND, DL, VT, Arg.getOperand(0),
-                         Arg.getOperand(1), Arg.getOperand(2), Arg.getOperand(3));
-    case X86ISD::FNMADD_RND:
-      return DAG.getNode(X86ISD::FMSUB_RND, DL, VT, Arg.getOperand(0),
-                         Arg.getOperand(1), Arg.getOperand(2), Arg.getOperand(3));
-    case X86ISD::FNMSUB_RND:
-      return DAG.getNode(X86ISD::FMADD_RND, DL, VT, Arg.getOperand(0),
-                         Arg.getOperand(1), Arg.getOperand(2), Arg.getOperand(3));
+    case X86ISD::FMADD:      NewOpcode = X86ISD::FNMSUB;     break;
+    case X86ISD::FMSUB:      NewOpcode = X86ISD::FNMADD;     break;
+    case X86ISD::FNMADD:     NewOpcode = X86ISD::FMSUB;      break;
+    case X86ISD::FNMSUB:     NewOpcode = X86ISD::FMADD;      break;
+    case X86ISD::FMADD_RND:  NewOpcode = X86ISD::FNMSUB_RND; break;
+    case X86ISD::FMSUB_RND:  NewOpcode = X86ISD::FNMADD_RND; break;
+    case X86ISD::FNMADD_RND: NewOpcode = X86ISD::FMSUB_RND;  break;
+    case X86ISD::FNMSUB_RND: NewOpcode = X86ISD::FMADD_RND;  break;
     }
   }
+  if (NewOpcode)
+    return DAG.getBitcast(OrigVT, DAG.getNode(NewOpcode, DL, VT,
+                                              Arg.getNode()->ops()));
+
   return SDValue();
 }
 
 static SDValue lowerX86FPLogicOp(SDNode *N, SelectionDAG &DAG,
                               const X86Subtarget &Subtarget) {
   EVT VT = N->getValueType(0);
   if (VT.is512BitVector() && !Subtarget.hasDQI()) {
     // VXORPS, VORPS, VANDPS, VANDNPS are supported only under DQ extention.
switch (N->getOpcode()) {
       case X86ISD::FANDN: IntOpcode = X86ISD::ANDNP; break;
     }
     SDValue IntOp = DAG.getNode(IntOpcode, dl, MVT::v8i64, Op0, Op1);
     return DAG.getBitcast(VT, IntOp);
   }
   return SDValue();
 }
 
-/// Returns true if the node \p N is FNEG(x) or FXOR (x, 0x80000000).
-bool isFNEG(const SDNode *N) {
-  if (N->getOpcode() == ISD::FNEG)
-    return true;
+static SDValue combineXor(SDNode *N, SelectionDAG &DAG,
+                                 TargetLowering::DAGCombinerInfo &DCI,
+                                 const X86Subtarget &Subtarget) {
+  if (SDValue Cmp = foldVectorXorShiftIntoCmp(N, DAG, Subtarget))
+    return Cmp;
 
-  if (N->getOpcode() == X86ISD::FXOR) {
-    unsigned EltBits = N->getSimpleValueType(0).getScalarSizeInBits();
-    SDValue Op1 = N->getOperand(1);
+  if (DCI.isBeforeLegalizeOps())
+    return SDValue();
 
-    auto isSignBitValue = [&](const ConstantFP *C) {
-      return C->getValueAPF().bitcastToAPInt() == APInt::getSignBit(EltBits);
-    };
+  if (SDValue RV = foldXorTruncShiftIntoCmp(N, DAG))
+    return RV;
 
-    // There is more than one way to represent the same constant on
-    // the different X86 targets. The type of the node may also depend on size.
-    //  - load scalar value and broadcast
-    //  - BUILD_VECTOR node
-    //  - load from a constant pool.
-    // We check all variants here.
-    if (Op1.getOpcode() == X86ISD::VBROADCAST) {
-      if (auto *C = getTargetConstantFromNode(Op1.getOperand(0)))
-        return isSignBitValue(cast<ConstantFP>(C));
+  if (Subtarget.hasCMov())
+    if (SDValue RV = combineIntegerAbs(N, DAG))
+      return RV;
 
-    } else if (BuildVectorSDNode *BV = dyn_cast<BuildVectorSDNode>(Op1)) {
-      if (ConstantFPSDNode *CN = BV->getConstantFPSplatNode())
-        return isSignBitValue(CN->getConstantFPValue());
+  if (SDValue FPLogic = convertIntLogicToFPLogic(N, DAG, Subtarget))
+    return FPLogic;
 
-    } else if (auto *C = getTargetConstantFromNode(Op1)) {
-      if (C->getType()->isVectorTy()) {
-        if (auto *SplatV = C->getSplatValue())
-          return isSignBitValue(cast<ConstantFP>(SplatV));
-      } else if (auto *FPConst = dyn_cast<ConstantFP>(C))
-        return isSignBitValue(FPConst);
-    }
-  }
-  return false;
+  if (isFNEG(N))
+    return combineFneg(N, DAG, Subtarget);
+  return SDValue();
 }
 
 /// Do target-specific dag combines on X86ISD::FOR and X86ISD::FXOR nodes.
 static SDValue combineFOr(SDNode *N, SelectionDAG &DAG,
                           const X86Subtarget &Subtarget) {
   assert(N->getOpcode() == X86ISD::FOR || N->getOpcode() == X86ISD::FXOR);
 
   // F[X]OR(0.0, x) -> x
static SDValue combineFMA(SDNode *N, SelectionDAG &DAG,
   EVT ScalarVT = VT.getScalarType();
   if ((ScalarVT != MVT::f32 && ScalarVT != MVT::f64) || !Subtarget.hasAnyFMA())
     return SDValue();
 
   SDValue A = N->getOperand(0);
   SDValue B = N->getOperand(1);
   SDValue C = N->getOperand(2);
 
-  bool NegA = isFNEG(A.getNode());
-  bool NegB = isFNEG(B.getNode());
-  bool NegC = isFNEG(C.getNode());
+  auto invertIfNegative = [](SDValue& V) {
+    SDValue NegVal = isFNEG(V.getNode());
+    if (NegVal) {
+      V = NegVal;
+      return true;
+    }
+    return false;
+  };
+
+  bool NegA = invertIfNegative(A);
+  bool NegB = invertIfNegative(B);
+  bool NegC = invertIfNegative(C);
 
   // Negative multiplication when NegA xor NegB
   bool NegMul = (NegA != NegB);
-  if (NegA)
-    A = A.getOperand(0);
-  if (NegB)
-    B = B.getOperand(0);
-  if (NegC)
-    C = C.getOperand(0);
 
   unsigned NewOpcode;
   if (!NegMul)
     NewOpcode = (!NegC) ? X86ISD::FMADD : X86ISD::FMSUB;
   else
     NewOpcode = (!NegC) ? X86ISD::FNMADD : X86ISD::FNMSUB;
 
   if (N->getOpcode() == X86ISD::FMADD_RND) {

../test/CodeGen/X86/fma-fneg-combine.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -mattr=+avx512bw -mattr=+avx512vl -mattr=+avx512dq  | FileCheck %s
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -mattr=+avx512bw -mattr=+avx512vl -mattr=+avx512dq  | FileCheck %s  --check-prefix=CHECK --check-prefix=SKX
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -mattr=+avx512f -mattr=+fma | FileCheck %s --check-prefix=CHECK --check-prefix=KNL
 
 ; This test checks combinations of FNEG and FMA intrinsics on AVX-512 target
 ; PR28892
 
 define <16 x float> @test1(<16 x float> %a, <16 x float> %b, <16 x float> %c)  {
 ; CHECK-LABEL: test1:
 ; CHECK:       # BB#0: # %entry
 ; CHECK-NEXT:    vfmsub213ps %zmm2, %zmm1, %zmm0

 ; CHECK-NEXT:    retq
 entry:
   %0 = tail call <8 x float> @llvm.x86.fma.vfmsub.ps.256(<8 x float> %a, <8 x float> %b, <8 x float> %c) #2
   %sub.i = fsub <8 x float> <float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00>, %0
   ret <8 x float> %sub.i
 }
 
 define <8 x float> @test8(<8 x float> %a, <8 x float> %b, <8 x float> %c) {
-; CHECK-LABEL: test8:
-; CHECK:       # BB#0: # %entry
-; CHECK-NEXT:    vxorps {{.*}}(%rip){1to8}, %ymm2, %ymm2
-; CHECK-NEXT:    vfmsub213ps %ymm2, %ymm1, %ymm0
-; CHECK-NEXT:    retq
+; SKX-LABEL: test8:
+; SKX:       # BB#0: # %entry
+; SKX-NEXT:    vxorps {{.*}}(%rip){1to8}, %ymm2, %ymm2
+; SKX-NEXT:    vfmsub213ps %ymm2, %ymm1, %ymm0
+; SKX-NEXT:    retq
+;
+; KNL-LABEL: test8:
+; KNL:       # BB#0: # %entry
+; KNL-NEXT:    vbroadcastss {{.*}}(%rip), %ymm3
+; KNL-NEXT:    vxorps %ymm3, %ymm2, %ymm2
+; KNL-NEXT:    vfmsub213ps %ymm2, %ymm1, %ymm0
+; KNL-NEXT:    retq
 entry:
   %sub.c = fsub <8 x float> <float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00>, %c
   %0 = tail call <8 x float> @llvm.x86.fma.vfmsub.ps.256(<8 x float> %a, <8 x float> %b, <8 x float> %sub.c) #2
   ret <8 x float> %0
 }
 
 declare <8 x float> @llvm.x86.fma.vfmsub.ps.256(<8 x float>, <8 x float>, <8 x float>)
 
 
 define <8 x double> @test9(<8 x double> %a, <8 x double> %b, <8 x double> %c) {
 ; CHECK-LABEL: test9:
 ; CHECK:       # BB#0: # %entry
 ; CHECK-NEXT:    vfnmsub213pd %zmm2, %zmm1, %zmm0
 ; CHECK-NEXT:    retq
 entry:
   %0 = tail call <8 x double> @llvm.x86.avx512.mask.vfmadd.pd.512(<8 x double> %a, <8 x double> %b, <8 x double> %c, i8 -1, i32 4) #2
   %sub.i = fsub <8 x double> <double -0.000000e+00, double -0.000000e+00, double -0.000000e+00, double -0.000000e+00, double -0.000000e+00, double -0.000000e+00, double -0.000000e+00, double -0.000000e+00>, %0
   ret <8 x double> %sub.i
 }
 
 declare <8 x double> @llvm.x86.avx512.mask.vfmadd.pd.512(<8 x double> %a, <8 x double> %b, <8 x double> %c, i8, i32)
 
-define <4 x double> @test10(<4 x double> %a, <4 x double> %b, <4 x double> %c) {
-; CHECK-LABEL: test10:
-; CHECK:       # BB#0: # %entry
-; CHECK-NEXT:    vfnmsub213pd %ymm2, %ymm1, %ymm0
-; CHECK-NEXT:    retq
-entry:
-  %0 = tail call <4 x double> @llvm.x86.avx512.mask.vfmadd.pd.256(<4 x double> %a, <4 x double> %b, <4 x double> %c, i8 -1) #2
-  %sub.i = fsub <4 x double> <double -0.000000e+00, double -0.000000e+00, double -0.000000e+00, double -0.000000e+00>, %0
-  ret <4 x double> %sub.i
-}
-
-declare <4 x double> @llvm.x86.avx512.mask.vfmadd.pd.256(<4 x double> %a, <4 x double> %b, <4 x double> %c, i8)
-
-define <2 x double> @test11(<2 x double> %a, <2 x double> %b, <2 x double> %c) {
-; CHECK-LABEL: test11:
-; CHECK:       # BB#0: # %entry
-; CHECK-NEXT:    vfnmsub213sd %xmm2, %xmm0, %xmm1
-; CHECK-NEXT:    vmovaps %xmm1, %xmm0
-; CHECK-NEXT:    retq
+define <2 x double> @test10(<2 x double> %a, <2 x double> %b, <2 x double> %c) {
+; SKX-LABEL: test10:
+; SKX:       # BB#0: # %entry
+; SKX-NEXT:    vfnmsub213sd %xmm2, %xmm0, %xmm1
+; SKX-NEXT:    vmovaps %xmm1, %xmm0
+; SKX-NEXT:    retq
+;
+; KNL-LABEL: test10:
+; KNL:       # BB#0: # %entry
+; KNL-NEXT:    vfnmsub213sd %xmm2, %xmm0, %xmm1
+; KNL-NEXT:    vmovaps %zmm1, %zmm0
+; KNL-NEXT:    retq
 entry:
   %0 = tail call <2 x double> @llvm.x86.avx512.mask.vfmadd.sd(<2 x double> %a, <2 x double> %b, <2 x double> %c, i8 -1, i32 4) #2
   %sub.i = fsub <2 x double> <double -0.000000e+00, double -0.000000e+00>, %0
   ret <2 x double> %sub.i
 }
 
 declare <2 x double> @llvm.x86.avx512.mask.vfmadd.sd(<2 x double> %a, <2 x double> %b, <2 x double> %c, i8, i32)