[PATCH] D98718: [AA][NFC] Convert AliasResult to class containing offset for PartialAlias case.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:140
 /// << operator for AliasResult.
 raw_ostream &operator<<(raw_ostream &OS, AliasResult AR);
 
----------------
dfukalov wrote:
> nikic wrote:
> > I think the AliasResult printing code should be adjusted to also print the PartialAlias offset. This will allow us to test this functionality using normal `-aa-eval` tests, instead of unit tests. That should be a great deal simpler when testing more complex cases.
> There is an issue with printing offset in `-aa-eval`: it prints two (aliased) values [[ https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Analysis/AliasAnalysisEvaluator.cpp$54 | sorted lexicographically ]].
> So the printed offset may have wrong sign regarding printed pair of values. I'm not sure how it should be printed to avoid confusion in tests... Do you have any ideas?
Would just calling swap() on the AliasResult if the values are swapped not work?


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1139
+        AliasResult AR = AliasResult::PartialAlias;
         if (VisitedPhiBBs.empty() && VRightSize.hasValue() &&
+            Off.ule(INT32_MAX) && (Off + VRightSize.getValue()).ule(LSize)) {
----------------
dfukalov wrote:
> nikic wrote:
> > Now that the offset is part of the result, I *think* the VisitedPhiBBs check is not needed anymore, but I'm not entirely sure.
> Actually I added this check because of [[ https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp$0-515 | comment ]] you asked about in D95543.
I think these are different issues. The comment refers to phi translation performed by MDA and GVN. BasicAA itself does not perform phi translation.

Here, VisitedPhiBBs should already implicit take effect, in that returning PartialAlias here requires that the GEP base pointers are MustAlias. They will only be MustAlias if they pass the phi cycle reachability check. So the VisitedPhiBBs are important for the recursive check on the base pointers, but there shouldn't be a need to check it here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98718