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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
