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

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

kpn added inline comments.


================
Comment at: include/llvm/IR/IntrinsicInst.h:235
+      ebStrict           ///< This corresponds to "fpexcept.strict".
     };
 
----------------
rjmccall wrote:
> 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>`.
The current code is in ConstrainedFPIntrinsic::getExceptionBehavior(), which returns ebInvalid if the metadata is missing or somehow wrong (wrong type, invalid string, etc). This is then used by the IR verifier which fails with an Assert() if it gets ebInvalid. So we aren't propagating ill-formed IR.

The intrinsic requires the metadata, and it requires the metadata be valid. But I don't want to clutter up IRBuilder's consumer's code. That's why I wanted the API to allow those arguments to not be given.

One idea of mine is to have ebInvalid be valid when given to the IRBuilder, and the meaning would be "this value is unspecified". But that seems weird.

Maybe there's another way.


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

https://reviews.llvm.org/D53157