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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert requested changes to this revision.
jdoerfert added a comment.
This revision now requires changes to proceed.

> That's basically how the optimization works. Behavior is undefined if the parameter is null, therefore we can assume it is not null, which unlock optimizations.

And I argue this is the problem, see below.

> We could presumably fix this in other ways, but my thinking is that not generating assumptions for undef values is a sensible thing to do.

I doubt this is a "fix" but simply hides the problem one level deeper. Think of a non-undef value for which this happens but after one iteration of inlining, constant propagation, etc. it is replaced by undef, thus in the assumption. I guess you could write a test for this situation rather easily.

---

I think the "solution" here is to weaken the guarantees of `nonnull`. That is, if the value is dead, it might be null after all. So `nonnull` should mean what `nonnull` means right now for any use of the value that affects a side effect (incl. control flow). Other attributes will need similar adjustment. This is in line with out of bounds `inbounds` geps, and similar situations.

FWIW, I had similar thoughts in the context of the attributes the Attributor deduces. They can sometimes be "contradicting" or "odd" but only for dead values.

---

My recommendation: Change the lang-ref and make the code that produces the assume only do it if we know the pointer is "known to be live".

---

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70749