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