rjmccall added inline comments.
================
Comment at: include/llvm/IR/IRBuilder.h:228
+ /// Enable/Disable use of constrained floating point math
+ void setIsConstrainedFP(bool IsCon) { IsFPConstrained = IsCon; }
+
----------------
kpn wrote:
> erichkeane wrote:
> > kpn wrote:
> > > kbarton wrote:
> > > > This is a minor quibble, but the method is setIsConstrainedFP, while
> > > > the member is IsFPConstrained.
> > > > I'm not sure if that is intentionally different, or an oversight.
> > > Yeah, that's an oversight. Fixed.
> > IS this fixed?
> In my working copy, yes.
Maybe this should be more explicit about what exactly it does? Specifically,
it changes the behavior of the `CreateF<Op>` methods so that they use
constrained intrinsics instead of the standard instructions.
================
Comment at: include/llvm/IR/IRBuilder.h:113
+ CR_ToZero ///< This corresponds to "fpround.tozero".
+ };
+
----------------
Should these have "FP" in the name somewhere? And are they really
IRBuilder-specific concepts, as opposed to something that should be declared as
part of the interface for working with these intrinsics?
Also, I believe we can use explicit underlying types now in LLVM; it'd be nice
if we didn't make `IRBuilder` unnecessarily large.
================
Comment at: include/llvm/IR/IRBuilder.h:255
+ /// Disable use of constrained floating point math
+ void clearIsFPConstrained() { setIsFPConstrained(false); }
+
----------------
This seems unnecessary.
================
Comment at: include/llvm/IR/IRBuilder.h:1138
+
+ return MetadataAsValue::get(Context, RoundingMDS);
+ }
----------------
Huh? You build an `MDNode` that wraps an `MDString` and then immediately
extract the `MDString` from it and drop the `MDNode`?
I think you should just have a function somewhere that returns the correct
metadata string for a `ConstrainedRoundingKind`, and then the code here is much
more obvious. Maybe this can be wherever you declare the enums. You can also
have a function that goes the other way and returns an
`Optional<ConstrainedRoundingKind>`.
Function name should include `FP`.
Same points apply to the next function.
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
