[PATCH] D89991: [AliasAnalysis] Fix a bug in alias analysis interaction with CSE w/ memorySSA

[Haoran Xu via Phabricator via llvm-commits]

haoranxu510 created this revision.
haoranxu510 added reviewers: asbirlea, chandlerc, sanjoy, hfinkel.
Herald added subscribers: llvm-commits, hiraditya, Prazek.
Herald added a project: LLVM.
haoranxu510 requested review of this revision.

I noticed that clang miscompiled a C code generated from brainfxxk-to-C transpiler
(https://godbolt.org/z/676Grr), under `-O1` or higher, a loop is wrongly optimized
into a dead loop.

After investigation, it turned out that the cause of the issue is that CSE w/
memorySSA transformation pass wrongly eliminated a `--(*ptr)` in the loop using
the result of another `--(*ptr)` outside the loop, without realizing that `*ptr`
may have been modified by the loop.

By bisecting the git history, it turned out that the bug was introduced by `D59315`.
The said diff seems to be a refactoring diff, however, it seems like the author
accidentally removed two lines of logic in refactoring, resulting in the bug.

I added the two lines back. While it solves the correctness issue, I'm not certain
on the performance implications of this change, since by clearing the AAQI after
each alias query, perhaps we would lose most of the gain from batching AA queries.

During the investigation, I also noticed that the author forgot to change a few
places to use his new API, however, I'm not sure if it is a correctness issue
or not.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89991

Files:
  llvm/lib/Analysis/AliasAnalysis.cpp
  llvm/lib/Analysis/BasicAliasAnalysis.cpp


Index: llvm/lib/Analysis/BasicAliasAnalysis.cpp
===================================================================
--- llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -859,6 +859,8 @@
   AliasResult Alias = aliasCheck(LocA.Ptr, LocA.Size, LocA.AATags, LocB.Ptr,
                                  LocB.Size, LocB.AATags, AAQI);
 
+  AAQI.AliasCache.shrink_and_clear();
+  AAQI.IsCapturedCache.shrink_and_clear();
   VisitedPhiBBs.clear();
   return Alias;
 }
Index: llvm/lib/Analysis/AliasAnalysis.cpp
===================================================================
--- llvm/lib/Analysis/AliasAnalysis.cpp
+++ llvm/lib/Analysis/AliasAnalysis.cpp
@@ -234,7 +234,7 @@
 
   // If Loc is a constant memory location, the call definitely could not
   // modify the memory location.
-  if (isModSet(Result) && pointsToConstantMemory(Loc, /*OrLocal*/ false))
+  if (isModSet(Result) && pointsToConstantMemory(Loc, AAQI, /*OrLocal*/ false))
     Result = clearMod(Result);
 
   return Result;
@@ -311,7 +311,7 @@
 
       // ModRefC1 indicates what Call1 might do to Call2ArgLoc, and we use
       // above ArgMask to update dependence info.
-      ModRefInfo ModRefC1 = getModRefInfo(Call1, Call2ArgLoc);
+      ModRefInfo ModRefC1 = getModRefInfo(Call1, Call2ArgLoc, AAQI);
       ArgMask = intersectModRef(ArgMask, ModRefC1);
 
       // Conservatively clear IsMustAlias unless only MustAlias is found.
@@ -352,7 +352,7 @@
       // might Mod Call1ArgLoc, then we care about either a Mod or a Ref by
       // Call2. If Call1 might Ref, then we care only about a Mod by Call2.
       ModRefInfo ArgModRefC1 = getArgModRefInfo(Call1, Call1ArgIdx);
-      ModRefInfo ModRefC2 = getModRefInfo(Call2, Call1ArgLoc);
+      ModRefInfo ModRefC2 = getModRefInfo(Call2, Call1ArgLoc, AAQI);
       if ((isModSet(ArgModRefC1) && isModOrRefSet(ModRefC2)) ||
           (isRefSet(ArgModRefC1) && isModSet(ModRefC2)))
         R = intersectModRef(unionModRef(R, ArgModRefC1), Result);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89991.300116.patch
Type: text/x-patch
Size: 2010 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201022/e4dd1d3c/attachment.bin>