[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3381
+                             Addend->getType()),
+        {MulOp0, MulOp1, Addend, MulOp->getOperand(2), MulOp->getOperand(3)});
+  else
----------------
You shouldn't just assume that MulOp is a constrained intrinsic. Cast to ConstrainedFPIntrinsic and use ConstrainedFPIntrinsic::getRoundingMode() and ConstrainedFPIntrinsic::getExceptionBehavior(). The cast will effectively assert that MulOp is a constrained intrisic. I think that should always be true.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3423
 
+  if (Builder.getIsFPConstrained()) {
+    if (auto *LHSBinOp = dyn_cast<llvm::CallBase>(op.LHS)) {
----------------
I don't think we should ever non-constrained create FMul instructions if Builder is in FP constrained mode, but you should assert that somewhere above. Maybe move this block above line 3409 and add:

assert(LHSBinOp->getOpcode() != llvm::Instruction::FMul && RHSBinOp->getOpcode() != llvm::Instruction::FMul);


================
Comment at: clang/test/CodeGen/constrained-math-builtins.c:160
+// CHECK: declare x86_fp80 @llvm.experimental.constrained.fmuladd.f80(x86_fp80, x86_fp80, x86_fp80, metadata, metadata)
+};
----------------
I'd like to see a test that verifies the calls generated in the function and specifically a test that verifies that the constrained fneg is generated if needed.


================
Comment at: llvm/docs/LangRef.rst:16094
+
+The fourth and fifth arguments specifie the exception behavior as described
+above.
----------------
s/specifie/specify

s/the exception behavior/the rounding mode and exception behavior


================
Comment at: llvm/docs/LangRef.rst:16104
+
+      %0 = call float @llvm.experimental.constrained.fmuladd.f32(%a, %b, %c)
+
----------------
missing metadata arguments


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:1515
              ConcreteTTI->getArithmeticInstrCost(BinaryOperator::FAdd, RetTy);
+    // FIXME: Is constrained intrinsic' cost equal to it's no strict one?
+    if (IID == Intrinsic::experimental_constrained_fmuladd)
----------------
I don't think that matters. The cost calculation here is a conservative estimate based on the cost if we are unable to generate an FMA instruction. So a constrained fmuladd that can't be lowered to FMA will be lower the same way a contrained mul followed by a constrained add would be.


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:355
 
+    /// FMULADD/STRICT_FMULADD - A intermediate node, made functions handle
+    /// constrained fmuladd the same as other constrained intrinsics.
----------------
Something is wrong with this comment. I'm not sure what it's trying to say but the grammar is wrong.

After looking through the rest of the code, I think I understand what's going on. I think we need a verbose comment to explain it. Here's my suggestion

```
FMULADD/STRICT_FMULADD -- These are intermediate opcodes used to handle the constrained.fmuladd intrinsic. The FMULADD opcode only exists because it is required for correct macro expansion and default handling (which is never reached). There should never be a node with ISD::FMULADD. The STRICT_FMULADD opcode is used to allow selectionDAGBuilder::visitConstrainedFPIntrinsic to determine (based on TargetOptions and target cost information) whether the constrained.fmuladd intrinsic should be lowered to FMA or separate FMUL and FADD operations.
```
Having thought through that, however, it strikes me as a lot of overhead. Can we just add special handling for the constrained.fmuladd intrinsic and make the decision then to create either a STRICT_FMA node or separate STRICT_FMUL and STRICT_FADD?

The idea that ISD::FMULADD is going to exist as a defined opcode but we never intend to add any support for handling it is particularly bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72820