[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/IntrinsicInst.h:235
+      ebStrict           ///< This corresponds to "fpexcept.strict".
     };
 
----------------
kpn wrote:
> 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.
> 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.

Okay, in that case, I don't understand why this accessor shouldn't just assume that it's working on valid IR and not be able to return `ebInvalid`.

> 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.

If you want the `IRBuilder` factories to be able to not take a value (if there's some defaulting scheme that's more complicated than you can express with a default argument), you can have it take an `Optional<Whatever>`, and then people can just pass in `None`; and then you don't need to muddy up the enum with a case that isn't really a valid alternative.


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

https://reviews.llvm.org/D53157