[PATCH] D36335: Add ‘llvm.experimental.constrained.fma‘ Intrinsic

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor requested changes to this revision.
andrew.w.kaylor added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6651
   case ISD::STRICT_FREM: NewOpc = ISD::FREM; break;
+  case ISD::STRICT_FMA: NewOpc = ISD::FMA; break;
   case ISD::STRICT_FSQRT: NewOpc = ISD::FSQRT; IsUnary = true; break;
----------------
b-sumner wrote:
> wdng wrote:
> > b-sumner wrote:
> > > This is a ternary operation.  Code below assumes unary or binary.
> > In Intrinsics.td, we have defined fma is a ternary operator. Here it only mutates STRICT_FMA to FMA and IsUnary is false by default. So we may not need to specify whether is unary of binary here?
> Please take a look at lines 6676 - 6680 below.  Do you not need to pass a 3 element list to MorphNodeTo for the FMA case?
You definitely need to add code to handle the third argument.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6015
   SDValue Result;
   if (FPI.isUnaryOp())
+    Result = DAG.getNode(Opcode, sdl, VTs,
----------------
This code also needs to be updated to handle the case of three value operands.


================
Comment at: lib/IR/Verifier.cpp:3985
   case Intrinsic::experimental_constrained_nearbyint:
     visitConstrainedFPIntrinsic(
         cast<ConstrainedFPIntrinsic>(*CS.getInstruction()));
----------------
The implementation of this function assumes only 1 or 2 value operands.  It will need to be updated.


================
Comment at: test/Feature/fp-intrinsics.ll:236
+; CHECK-LABEL: f17
+; CHECK: call double @llvm.experimental.constrained.fma
+define double @f17() {
----------------
If you checked the arguments here it should reveal the problems in the code. 

There's also a test at llvm/tests/CodeGen/X86/fp-intrinsics.ll that carries the constrained FP intrinsics all the way through code generation.  Can you add a case there for this intrinsic?


Repository:
  rL LLVM

https://reviews.llvm.org/D36335