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

[Evgenii Kudriashov via Phabricator via llvm-commits]

e-kud 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
----------------
pengfei wrote:
> Do we need to check it again given it's the only condition checked in line 54073?
> Do we need to check it again given it's the only condition checked in line 54073?

Dropped it.


================
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()) {
----------------
pengfei wrote:
> This doesn't explain why we need to iterate all its uses?
> This doesn't explain why we need to iterate all its uses?

I added comments describing the approach. Generally we don't want to optimize anything if there are other non-FMA consumers of constant `BUILD_VECTOR`. Because it will be loaded anyway. So, we want to understand which one (inverted or original) can be eliminated. If both can be eliminated, we need to fix a particular vector because we don't have a context to understand which one is original and which one is inverted even among a single instruction operands.

If there is a concern about complexity of this check, it can be replaced with simplier `hasOneUse`. It limits the scope though.


================
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));
----------------
pengfei wrote:
> Maybe use `cast`?
> Maybe use `cast`?




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

> Any case can fall into here?

This is completely OK to have a constant vector with `undef` values, e.g `<2 x double> <double undef, double undef>`. There are some tests with such vectors. So, we can't use `cast` here. I assumed that during optimization we may obtain such the constant.


================
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.
----------------
pengfei wrote:
> Is the comment for line 54015? Why checking the use again?
I've described approach above. Here we check users of an inverted vector.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146494