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

[Daniil Fukalov via Phabricator via llvm-commits]

dfukalov added inline comments.


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:140
 /// << operator for AliasResult.
 raw_ostream &operator<<(raw_ostream &OS, AliasResult AR);
 
----------------
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?


================
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)) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98718