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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:259
+    return DefaultConstrainedRounding.getValue();
+  }
+
----------------
kpn wrote:
> 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. 
Okay, that scheme makes sense, but I think sanity-checking that there hasn't been an arbitrary memory smasher is a bit much.  Using `Optional` when the value can't actually be missing just makes the code more confusing for readers and maintainers; please just leave these non-optional.


================
Comment at: include/llvm/IR/IRBuilder.h:1324
+      return CreateConstrainedFPBinOp(Intrinsic::experimental_constrained_fadd,
+                                      L, R, nullptr, Name);
+
----------------
kpn wrote:
> 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.
Okay.  That should probably be mentioned, at least in the documentation for `CreateConstrainedFPBinOp`.  Should you make an overload of the latter which takes an `MDNode*` as the final argument, for parallelism/completeness?


================
Comment at: include/llvm/IR/IRBuilder.h:1459
+      Optional<ConstrainedFPIntrinsic::RoundingMode> Rounding = None,
+      Optional<ConstrainedFPIntrinsic::ExceptionBehavior> Except = None) {
+    Value *RoundingV = getConstrainedFPRounding(Rounding);
----------------
kpn wrote:
> 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.
Okay.  So people who want to pass these explicitly just can't use the convenience methods?  That seems like a reasonable compromise.


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

https://reviews.llvm.org/D53157