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

Sun Jan 26 21:19:19 PST 2020

pengfei marked an inline comment as done.
pengfei added inline comments.

================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3381
+           "constrained mode");
+    FMulAdd = Builder.CreateCall(
+        CGF.CGM.getIntrinsic(llvm::Intrinsic::experimental_constrained_fmuladd,
----------------
kpn wrote:
> craig.topper wrote:
> > kpn wrote:
> > > craig.topper wrote:
> > > > Doesn't this need to be CreateConstrainedFPCall so that the strictfp attribute is added? That will take care of adding the metadata operands too.
> > > Is this code tested? I ran into a bug yesterday where CreateCall was used with a constrained intrinsic and the Instruction class blew up because the function signature was wrong. I wasn't passing in the metadata arguments.
> > > 
> > > So, yes, it should be, and it would might make sense for the patch to have test coverage that catches any other cases of this.
> > This code is copying the metadata arguments from the fmul intrinsic, MulOp. that’s the getOperand(2) and getOperand(3).
> Ah, yes, thanks. Your comment about the attribute is still valid, though. And, yes, using CreateConstrainedFPCall() is the easiest way to fix the attribute.
Yes, it's the best choice. Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72820