[PATCH] D28087: X86 instr selection: combine ADDSUB + MUL to FMADDSUB

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Thanks for looking at this - I created PR30633 which talked about combining to vfmaddsub/vfmsubadd from buildvector/shuffles of vfmaddadd/vfmaddsub inputs as well, which is likely to happen if the combiner matches individual scalar FMAs first.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:32048
+  SDValue Op1 = N->getOperand(0);
+  if (N->getOpcode() != X86ISD::ADDSUB || Op1->getOpcode() != ISD::FMUL ||
+      !Op1->hasOneUse() || !Subtarget.hasFMA())
----------------
You should probably add an assert for the ADDSUB opcode - if this got called its really gone wrong!

```
assert(N->getOpcode() == X86ISD::ADDSUB && "Expected X86ISD::ADDSUB opcode");
```


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:32049
+  if (N->getOpcode() != X86ISD::ADDSUB || Op1->getOpcode() != ISD::FMUL ||
+      !Op1->hasOneUse() || !Subtarget.hasFMA())
+    return SDValue();
----------------
Use Subtarget.hasAnyFMA() so FMA4 works as well


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:32052
+
+  const TargetOptions &Options = DAG.getTarget().Options;
+  bool AllowFusion =
----------------
Add a comment explaining that this must match the FMA combine logic in DAGCombiner::visitFADDForFMACombine


================
Comment at: llvm/test/CodeGen/X86/fmaddsub-combine.ll:2
+
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+
----------------
Please regenerate this with utils\update_llc_test_checks.py and add -mattr=+avx2,+fma to the llc command, possibly add a second pass with -mattr=+avx,+fma4 as well.

Please can you add tests with the @llvm.x86.sse3.addsub (and avx equivalent) pd/ps intrinsics.

The existing test looks like it can be simplified as well, if you wish to check for load folding add them as separate tests.



https://reviews.llvm.org/D28087