[PATCH] D70749: [InstCombine] do not insert nonnull assumption for undef

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D70749#1762374 <https://reviews.llvm.org/D70749#1762374>, @efriedma wrote:

> In D70749#1762291 <https://reviews.llvm.org/D70749#1762291>, @jdoerfert wrote:
>
> > >> The only alternative I see is to make passes that replace dead values aware of attributes, e.g., let them clear them out. Though, I fear that also hides the problems only a few indirections deeper.
> > > 
> > > I don't think there's some deep class of problems hidden here beyond DeadArgElim itself.  The current rule is clear.  I would rather not change LangRef unless there's some interesting optimization that's blocked by the current semantics.
> >
> > Every time we mix unused values with annotated information/attributes it is problematic. Take a function that returns a value but we know the return value is never inspected at any call site. We replace it with undef regardless of what the function and call site return arguments are.
> >  Now we are looking for a mechanism to annotate information/attributes in the IR even if they are not arguments (see https://reviews.llvm.org/D37579#1761151) which will expose this mismatch not only for arguments and return values.
>
>
> For most optimizations, a use in a "call" or "ret" instruction is a real use, enough to force optimizations to assume that other code depends on the value.  There's only a narrow class of IPO optimizations (I think just DeadArgElim and IPSCCP) which prove an argument or return value is "dead" despite having a use in a call or ret instruction.
>
> There's a corresponding rule for "!nonnull", in LangRef: "If the value is null at runtime, the behavior is undefined."  Code that hoists load instructions will erase the metadata.  This works correctly, as far as I know.


We are consistent by calling everything quickly UB, does not mean that this is the right choice. I would expect a transformation in the code base that does not erase the metadata or a bug report that exposed such behavior (because I would not have known to do it right now). This is again one of those situations where we force people to be always aware of implicit interactions in order to produce correct transformations. Having it result in poison would help us here and in the situations I mentioned in my last comment. Could you summarize the drawbacks? (I guess we should move this to the list.)

> More generally, there are many places in IR that have UB without an associated side-effect.  For example, "sdiv" is UB if you divide by zero, or the result overflows.  I don't think there's anything fundamentally wrong with that.

I agree with the last part. Having UB without a side-effect is fine. Having UB for these argument attributes (and similar situations) is what I think we need to fix. I want us to have more fine grained control here. If you want to model UB at a location, we can still introduce an `llvm.assume(false)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70749