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

[Elena Demikhovsky via Phabricator via llvm-commits]

delena added a comment.

In https://reviews.llvm.org/D28087#636693, @v_klochkov wrote:

> This is my first experience of work on SDNodes..., please fix me if I am wrong..., but SDNodes do not have FastMathFlags, right?


See SDNodeFlags structure. 
const SDNodeFlags *Flags = &cast<BinaryWithFlagsSDNode>(N)->Flags;
But I don't know whether it is criteria for FMA. If yes, it should be checked. If not, the "fast" should be removed from the tests.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:7096
+/// FMADDSUB is.
+static bool IsFMAddSub(const X86Subtarget &Subtarget, SelectionDAG &DAG,
+                       SDValue &Opnd0, SDValue &Opnd1, SDValue &Opnd2) {
----------------
Function name starts from lowercase. Same bellow.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27851
+static bool IsAddSub(SDNode *N, const X86Subtarget &Subtarget,
+                     SelectionDAG &DAG, SDValue &Opnd0, SDValue &Opnd1) {
+
----------------
DAG is not used.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27910
+  SDValue Opnd0, Opnd1;
+  if (!IsAddSub(N, Subtarget, DAG, Opnd0, Opnd1))
+    return SDValue();
----------------
You consider the situation when the shuffle is resolved before FMA.
But if fadd and fsub are already combined with fmul, fmaddsub will not be created. I'm not sure that this situation is real, except fma are coming form intrinsics ..


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:27924
+  // X86 targets with 512-bit ADDSUB instructions!
+  if (VT == MVT::v16f32 || VT == MVT::v8f64)
     return SDValue();
----------------
VT.is512BitVector()


https://reviews.llvm.org/D28087