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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:110
       : Context(context), DefaultFPMathTag(FPMathTag),
-        DefaultOperandBundles(OpBundles) {
+        IsFPConstrained(false), DefaultConstrainedExcept(nullptr),
+        DefaultConstrainedRounding(nullptr), DefaultOperandBundles(OpBundles) {
----------------
Instead of adding these here, make these inline initializers on lines 100-102.


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


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


================
Comment at: include/llvm/IR/IRBuilder.h:1259
+                                  Instruction *FMFSource = nullptr,                                               const Twine &Name = "",
+                                  MDNode *RoundingMD = nullptr,               
+                                  MDNode *ExceptMD = nullptr) {
----------------
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?


================
Comment at: include/llvm/IR/IRBuilder.h:1408
 
+  CallInst *CreateConstrainedFRem(Value *L, Value *R, 
+                                  Instruction *FMFSource = nullptr, 
----------------
All these CreateConstrainedXXX should be distilled down to a single function that takes the intrinsic as a parameter.


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

https://reviews.llvm.org/D53157