[PATCH] D146494: [X86] Combine constant vector inputs for FMA

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53984-53985
+static SDValue getInvertedVectorForFMA(SDValue V, SelectionDAG &DAG) {
+  assert(ISD::isBuildVectorOfConstantFPSDNodes(V.getNode()) &&
+         "ConstantFP build vector expected");
+  // Check if we can eliminate a constant completely
----------------
Do we need to check it again given it's the only condition checked in line 54073?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53986
+         "ConstantFP build vector expected");
+  // Check if we can eliminate a constant completely
+  for (const SDNode *User : V->uses()) {
----------------
This doesn't explain why we need to iterate all its uses?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53988
+  for (const SDNode *User : V->uses()) {
+    if (User->getOpcode() != ISD::FMA || User->getOpcode() == ISD::STRICT_FMA)
+      return SDValue();
----------------
`==` is intended? Shouldn't it be covered by `User->getOpcode() != ISD::FMA`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53996
+  for (auto op : V->op_values()) {
+    if (auto *Cst = dyn_cast<ConstantFPSDNode>(op)) {
+      Ops.push_back(DAG.getConstantFP(-Cst->getValueAPF(), SDLoc(op), EltVT));
----------------
Maybe use `cast`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53999-54000
+    } else {
+      assert(op.isUndef());
+      Ops.push_back(DAG.getUNDEF(EltVT));
+    }
----------------
Any case can fall into here?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:54008-54010
+  // If the inverted value also can be eliminated, we have to persistancy
+  // prefer one of the values. We prefer a constant with negative value on the
+  // first element.
----------------
Is the comment for line 54015? Why checking the use again?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:54012
+  for (const SDNode *User : NV->uses())
+    if (User->getOpcode() != ISD::FMA || User->getOpcode() == ISD::STRICT_FMA)
+      return SDValue(NV, 0);
----------------
ditto.


================
Comment at: llvm/test/CodeGen/X86/fma-fneg-combine-2.ll:132
 ; FMA3:       # %bb.0:
-; FMA3-NEXT:    vmovapd {{.*#+}} ymm1 = [5.0E-1,5.0E-1,5.0E-1,5.0E-1]
-; FMA3-NEXT:    vfmadd213pd {{.*#+}} ymm0 = (ymm1 * ymm0) + mem
+; FMA3-NEXT:    vmovapd {{.*#+}} ymm1 = [-5.0E-1,-5.0E-1,-5.0E-1,-5.0E-1]
+; FMA3-NEXT:    vfnmadd213pd {{.*#+}} ymm0 = -(ymm1 * ymm0) + ymm1
----------------
We can make the elements different now, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146494