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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
