[PATCH] D127202: [InlineFunction] Call to getUnderlyingObjects inside AddAliasScopeMetadata shall set MaxLookup = 0

[Ting Wang via Phabricator via llvm-commits]

tingwang added a comment.

In D127202#3565819 <https://reviews.llvm.org/D127202#3565819>, @nikic wrote:

> Okay, I think the core problem here is that the code assumes that the underlying object may be either a) an argument, b) an identified function local object or c) an escape source -- in the sense of only being able to alias a noalias argument if it has been captured earlier. This assumption //does// mostly hold up in practice, because the implementations of getUnderlyingObjects() and CaptureTracking are carefully synchronized (see in particular https://github.com/llvm/llvm-project/blob/20ca739701d7b2e2aa4028f1a7853f6d0fa0c412/llvm/lib/Analysis/ValueTracking.cpp#L4446-L4458). Looking through history, I think it might be synchronized for this specific usage actually. Of course, the assumption doesn't hold up if we stop walking underlying objects early, in which case we may alias without the argument being captured.
>
> So this can be fixed in two ways: Avoid the assumption, or avoid stopping the walk early. For the latter case, something to be careful about is that using `MaxLookup=0` also removes infinite loop protection for unreachable code. You'll want to add a test case for that. Likely we'd actually want something like D86669 <https://reviews.llvm.org/D86669> as the implementation, which does avoid infinite loops.

Looks like we need some more check on results from getUnderlyingObjects(). @efriedma proposed check like `all_of(Objects, isIdentifiedObject)`, however there are cases for which object is not able to be identified, not due to the MaxLookup limit. I'm thinking maybe we need to know if walk stopped early or not in AddAliasScopeMetadata() to better handle this situation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127202