[PATCH] D143129: [GVN] Do not propagate equalities for noalias pointers

Tue Feb 14 01:39:42 PST 2023

nikic added a comment.

Thanks a lot for the detailed analysis!

In D143129#4117091 <https://reviews.llvm.org/D143129#4117091>, @mnadeem wrote:

> @nikic 
> I was looking at https://llvm.org/devmtg/2018-04/slides/Lopes-Sbirlea-Pointers,%20Alias%20and%20ModRef%20Analyses.pdf page 15, which says that replacement with null ptr is safe. So I did two runs, one where we do pointer replacement only when they have same underlying objects, and the another where we also replace pointers with null.

I believe the currently accepted interpretation of null in LLVM is that it has nullary provenance. For example, we rely on this here: https://github.com/llvm/llvm-project/blob/46cdf7f0991280a3601a777a80fbc460196fb033/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp#L1064 This folds `load (gep null, x)` to unreachable. Of course, we also fold `inttoptr 0` to `null`, which is inconsistent with that interpretation.

As @nlopes suggested, this should probably be discussed on discourse. Giving null universal provenance seems wrong from a principled perspective, but it may well be the most pragmatic option. (I don't think we rely on nullary provenance in many places, not sure there are any beyond the one I shared, and for that one we can probably recover most practically useful cases with an inbounds requirement.)

> Here are the diffs for llvm-test-suite bitcode:
> https://github.com/UsmanNadeem/vigilant-octo-journey/commit/125698f591df1f8de81ad4119b0838e135a18af8
> and
> https://github.com/UsmanNadeem/vigilant-octo-journey/commit/a778479334852e7f05b5a248da4473106aa986fb

Oh, that's a nice way to analyze differences. How did you produce the output?

I looked through the diffs a bit, and while most of them are immaterial diffs (just replacing one pointer with another without further simplification), it looks like there are also many genuine regressions in optimization quality. The first one is https://github.com/UsmanNadeem/vigilant-octo-journey/commit/a778479334852e7f05b5a248da4473106aa986fb#diff-429f0aa0b366d469c8f0fe4aaded9e87e83898118f192b6741caf4100e4e7792. I think the logic here is that we have a `icmp eq ptr %link.112821, %48` condition on the edge, and then check

  %313 = phi ptr [ %.pre12846, %for.end11932.loopexit ], [ %link.112821, %if.end11920 ]
  %cmp11936 = icmp ne ptr %313, %48

so if we replace `%link.112821` with `%48` here, we see that the condition is always true when coming from `%if.end11920` and that allows us to thread the branch. This is a legal optimization, because icmp is provenance independent.

>From a quick glance, the same basic pattern also occurs in many other IR diffs, so it seems like this is something important to preserve, though I'm not entirely sure what the best way to only propagating the equality in provenance-independent contexts would be (doing a recursive user walk seems rather iffy, but I don't have a better idea).

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

https://reviews.llvm.org/D143129