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

[Bob Haarman via Phabricator via llvm-commits]

inglorion added a comment.

Responding to @jdoerfert's comments:

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

I thought along those same lines. Since we replace values we have deduced to be unused with undef, my change is to only emit the assume for values that are not undef.

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

My take on this is that the transformation that generates a non-null assumption for a non-undef value is correct: if the value is live, it must be nonnull. As for later passes replacing the value with undef, my reading of the langref is that this would be incorrect. Specifically,
https://llvm.org/docs/LangRef.html#llvm-assume-intrinsic states "Note that the optimizer might limit the transformations performed on values used by the llvm.assume intrinsic in order to preserve the instructions only used to form the intrinsic’s input argument." which suggests to me the intent that values passed by llvm.assume be preserved. In other words, you can't just replace those by undef. So I don't think generating llvm.assumes for non-undef values creates a problem; simply not generating assumes for undefs should be enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70749