[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Kevin P. Neal via Phabricator via cfe-commits]

kpn marked 3 inline comments as done.
kpn added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:259
+    return DefaultConstrainedRounding.getValue();
+  }
+
----------------
rjmccall wrote:
> Okay, so what are the invariants here now?  It looks like, in order to enable constrained floating point, I need to also have set the default exception behavior and rounding mode.  That should at least be documented, but maybe a better approach would be to require these to be passed in when enabling constrained FP.
The IRBuilder constructor sets the defaults to ebStrict and rmDynamic, but leaves strict mode off. So if you only turn on strict mode you get the strictest settings.

This implies that it isn't possible to trigger this assertion. Which is true unless you have some form of memory overwrite or someone comes along later and puts in a bad IRBuilder change.

The more compiler work I've done over the years the more sanity checks I've gotten into the habit of adding. 


================
Comment at: include/llvm/IR/IRBuilder.h:1324
+      return CreateConstrainedFPBinOp(Intrinsic::experimental_constrained_fadd,
+                                      L, R, nullptr, Name);
+
----------------
rjmccall wrote:
> `FPMD` is dropped in this case; I don't know if that's intentional.
You can see that I'm on the fence here. I'm not sure that mixing fast math with constrained math makes sense. So CreateConstrainedFPBinOp() can take an instruction for copying fast math flags, but it doesn't do anything with this instruction. And the FPMD is simply dropped in the non-*FMF() methods.

If we do decide later to support mixing constrained and fast math then we won't have to change any APIs. Until then it takes the conservative route and drops the info.


================
Comment at: include/llvm/IR/IRBuilder.h:1459
+      Optional<ConstrainedFPIntrinsic::RoundingMode> Rounding = None,
+      Optional<ConstrainedFPIntrinsic::ExceptionBehavior> Except = None) {
+    Value *RoundingV = getConstrainedFPRounding(Rounding);
----------------
rjmccall wrote:
> It looks like nothing in `IRBuilder` ever passes these arguments.  Are you just anticipating that someone might want to call this directly?
Correct. And I wrote tests for it in IRBuilderTest.cpp.


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

https://reviews.llvm.org/D53157