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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70749