[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/IntrinsicInst.h:235
+      ebStrict           ///< This corresponds to "fpexcept.strict".
     };
 
----------------
kpn wrote:
> rjmccall wrote:
> > Is it okay that `ebUnspecified` and `ebInvalid` overlap here?
> I can think of a couple of alternatives. If they don't overlap then we have to go back and sweep the source to make sure that ebUnspecified is always handled in all cases that currently handle ebInvalid. And in the future nobody is allowed to check in source that doesn't handle both.
> 
> Or, we could drop ebUnspecified, but then ebInvalid would have a valid meaning to the IRBuilder interface. That looks like a bug even if it works properly.
> 
> Generally, adding eb/rmUnspecified but having them overlap the invalid cases seems to me to be the best option.
What is the purpose of `ebInvalid`?  Is this a valid argument to the intrinsic, or is it there so that `getExceptionBehavior()` has something to return if it doesn't recognize the argument?  For the latter, maybe it would be better to just assert that the argument was one of the recognized possibilities; we don't generally expect LLVM to silently propagate ill-formed IR.  Or if it's valid to not provide exception behavior to the intrinsic, maybe that's the same as `ebUnspecified`, or maybe the accessor should return `Optional<ExceptionBehavior>`.


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

https://reviews.llvm.org/D53157