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

[Kevin P. Neal via Phabricator via cfe-commits]

kpn added a comment.

I wanted to get the API straight before working on documentation.



================
Comment at: include/llvm/IR/IRBuilder.h:228
+  /// Enable/Disable use of constrained floating point math
+  void setIsConstrainedFP(bool IsCon) { IsFPConstrained = IsCon; }
+
----------------
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.


================
Comment at: include/llvm/IR/IRBuilder.h:234
+  /// Set the exception handling to be used with constrained floating point
+  void setDefaultConstrainedExcept(MDNode *NewExcept) { 
+    DefaultConstrainedExcept = NewExcept; 
----------------
mibintc wrote:
> andrew.w.kaylor wrote:
> > I think it would be better to add some enumerated type to describe the exception semantic and rounding modes. The MDNode is an implementation detail and an awkward one for the front end to have to deal with.
> I posted a patch showing the rounding and exception information being passed as enumeration not MDNode. I've uploaded the patch here, https://reviews.llvm.org/D62730
It's a good idea. D62730 beat me to the punch.


================
Comment at: include/llvm/IR/IRBuilder.h:1249
+    if (IsFPConstrained)
+      return CreateConstrainedFAdd(L, R, FMFSource, Name, nullptr, nullptr);
+
----------------
erichkeane wrote:
> Why set the last 2 to nullptr when you have defaults for these?
Eh, no really good reason.


================
Comment at: include/llvm/IR/IRBuilder.h:1259
+                                  Instruction *FMFSource = nullptr,                                               const Twine &Name = "",
+                                  MDNode *RoundingMD = nullptr,               
+                                  MDNode *ExceptMD = nullptr) {
----------------
erichkeane wrote:
> The last 2 parameters are never actually used except in the test.  Are these really important to have if they are never used in source?
They're part of the intrinsics. It seems like there should be a way to emit them as part of the instruction when creating the instruction.


================
Comment at: include/llvm/IR/IRBuilder.h:1408
 
+  CallInst *CreateConstrainedFRem(Value *L, Value *R, 
+                                  Instruction *FMFSource = nullptr, 
----------------
erichkeane wrote:
> All these CreateConstrainedXXX should be distilled down to a single function that takes the intrinsic as a parameter.
That moves us further down the road towards having modes. I haven't seen a front-end guy sign off on it yet, but it seems like a good idea.


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

https://reviews.llvm.org/D53157