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

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:113
+    CR_ToZero       ///< This corresponds to "fpround.tozero".
+  };
+
----------------
kpn wrote:
> rjmccall wrote:
> > 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.
> Would it be better to use the RoundingMode and ExceptionBehavior enums in the ConstrainedFPIntrinsic class? These enums->strings here get turned back into those IntrinsicInst.h enums eventually anyway. But that means pulling in yet more headers in IRBuilder.h.
> 
> I admit I'm not sure what you mean with your second paragraph. Is that a way of saying that, for example, the relevant IntrinsicInst.h enums should be used instead?
I think it's `IRBuilder.h`'s fate to pull in the majority of the headers in `IR`, but even if we want to avoid that, duplicating the enums seems like a step too far.  If there's too much code in the header declaring that intrinsic, you should extract a smaller header that just declares the enums and their string conversions.

The second paragraph is asking for these enums to be specifically constrained to `uint8_t` so that we aren't wasting a ton of memory everywhere we store them.  `IRBuilder` doesn't have strong size constraints, but it'd still be nice if all these accumulated features didn't make it hundreds of bytes larger than it needs to be.


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

https://reviews.llvm.org/D53157