[PATCH] D90066: [BasicAA] Fix caching in the presence of phi cycles

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added a reviewer: asbirlea.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.
nikic requested review of this revision.

Any time we insert a block into VisitedPhiBBs, previously cached values may no longer be valid for the recursive alias queries. As such, perform them using an empty AAQueryInfo. (Note that if we recurse to the same phi, the block will already be inserted, so we reuse the old AAQueryInfo, and thus still protect against infinite recursion.)

This is a different approach to fixing this problem than D90062 <https://reviews.llvm.org/D90062>, I'm not sure what the tradeoffs are yet.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90066

Files:
  llvm/lib/Analysis/BasicAliasAnalysis.cpp
  llvm/unittests/Analysis/AliasAnalysisTest.cpp


Index: llvm/unittests/Analysis/AliasAnalysisTest.cpp
===================================================================
--- llvm/unittests/Analysis/AliasAnalysisTest.cpp
+++ llvm/unittests/Analysis/AliasAnalysisTest.cpp
@@ -249,12 +249,12 @@
   auto &AA = getAAResults(*F);
   EXPECT_EQ(NoAlias, AA.alias(A1Loc, A2Loc));
   EXPECT_EQ(MayAlias, AA.alias(PhiLoc, A1Loc));
-  EXPECT_EQ(NoAlias, AA.alias(S1Loc, S2Loc)); // TODO: This is wrong
+  EXPECT_EQ(MayAlias, AA.alias(S1Loc, S2Loc));
 
   BatchAAResults BatchAA(AA);
   EXPECT_EQ(NoAlias, BatchAA.alias(A1Loc, A2Loc));
-  EXPECT_EQ(NoAlias, BatchAA.alias(PhiLoc, A1Loc)); // TODO: This is wrong.
-  EXPECT_EQ(NoAlias, BatchAA.alias(S1Loc, S2Loc)); // TODO: This is wrong
+  EXPECT_EQ(MayAlias, BatchAA.alias(PhiLoc, A1Loc));
+  EXPECT_EQ(MayAlias, BatchAA.alias(S1Loc, S2Loc));
 }
 
 class AAPassInfraTest : public testing::Test {
Index: llvm/lib/Analysis/BasicAliasAnalysis.cpp
===================================================================
--- llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -1655,14 +1655,20 @@
   // In the recursive alias queries below, we may compare values from two
   // different loop iterations. Keep track of visited phi blocks, which will
   // be used when determining value equivalence.
-  auto Pair = VisitedPhiBBs.insert(PN->getParent());
+  bool BlockInserted = VisitedPhiBBs.insert(PN->getParent()).second;
   auto _ = make_scope_exit([&]() {
-    if (Pair.second)
+    if (BlockInserted)
       VisitedPhiBBs.erase(PN->getParent());
   });
 
+  // If we inserted a block into VisitedPhiBBs, alias analysis results that
+  // have been cached earlier may no longer be valid. Perform recursive queries
+  // with a new AAQueryInfo.
+  AAQueryInfo NewAAQI;
+  AAQueryInfo *UseAAQI = BlockInserted ? &NewAAQI: &AAQI;
+
   AliasResult Alias = aliasCheck(V2, V2Size, V2AAInfo, V1Srcs[0], PNSize,
-                                 PNAAInfo, AAQI, UnderV2);
+                                 PNAAInfo, *UseAAQI, UnderV2);
 
   // Early exit if the check of the first PHI source against V2 is MayAlias.
   // Other results are not possible.
@@ -1678,8 +1684,8 @@
   for (unsigned i = 1, e = V1Srcs.size(); i != e; ++i) {
     Value *V = V1Srcs[i];
 
-    AliasResult ThisAlias =
-        aliasCheck(V2, V2Size, V2AAInfo, V, PNSize, PNAAInfo, AAQI, UnderV2);
+    AliasResult ThisAlias = aliasCheck(V2, V2Size, V2AAInfo, V, PNSize,
+                                       PNAAInfo, *UseAAQI, UnderV2);
     Alias = MergeAliasResults(ThisAlias, Alias);
     if (Alias == MayAlias)
       break;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D90066.300364.patch
Type: text/x-patch
Size: 2621 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201023/3a32e798/attachment.bin>