[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: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