[PATCH] D109953: [X86][FP16] Combine the FADD(A, FMA(B, C, 0)) to FMA(B, C, A)

[LiuChen via Phabricator via llvm-commits]

LiuChen3 added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47582
+    if ((Opcode == X86ISD::VFMADDC || Opcode == X86ISD::VFCMADDC) &&
+        ((ISD::isBuildVectorAllZeros(Op0->getOperand(0).getNode()) &&
+          AllowContract(Op0->getFlags())) ||
----------------
pengfei wrote:
> Should this be
> ```
> AllowContract(Op0->getFlags()) && (ISD::isBuildVectorAllZeros(Op0->getOperand(0).getNode()) && Op0->getFlags().hasNoSignedZeros()) || IsVectorAllNegativeZero(Op0->getOperand(0).getNode()))
> ```
> I.e, check `AllowContract` together with `IsVectorAllNegativeZero` as well.
AllowContract will check hasNoSignedZeros(). It seems that we can only do this combination when the fast-math flag is set, No matter if the third operand is +0.0 or 0.0. 
+0.0 or -0.0 affects the conversion of FMA(a, b, ±0.0) to FMUL(a, b).


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47612-47621
 static SDValue combineFaddFsub(SDNode *N, SelectionDAG &DAG,
-                               const X86Subtarget &Subtarget) {
-  if (SDValue HOp = combineToHorizontalAddSub(N, DAG, Subtarget))
-    return HOp;
+                             const X86Subtarget &Subtarget) {
+if (SDValue HOp = combineToHorizontalAddSub(N, DAG, Subtarget))
+  return HOp;
 
-  if (SDValue COp = combineFaddCFmul(N, DAG, Subtarget))
-    return COp;
+if (SDValue COp = combineFaddCFmul(N, DAG, Subtarget))
+  return COp;
----------------
pengfei wrote:
> This seems been changed unconsciously.
Sorry for this. Looks like I accidentally do some change here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109953/new/

https://reviews.llvm.org/D109953