[PATCH] D91383: [BasicAA] Make alias GEP positive offset handling symmetric

[Nikita Popov via Phabricator via llvm-commits]

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

aliasGEP() currently implements some special handling for the case where all variable offsets are positive, in which case the constant offset can be taken as the minimal offset. However, it does not perform the same handling for the all-negative case. This means that the alias-analysis result between two GEPs is asymmetric: If GEP1 - GEP2 is all-positive, then GEP2 - GEP1 is all-negative, and the first will result in NoAlias, while the second will result in MayAlias.

Apart from producing sub-optimal results for one order, this also violates our caching assumption. In particular, if BatchAA is used, the cached result depends on the order of the GEPs in the first query. This results in an inconsistency in BatchAA and AA results, which is how I noticed this issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91383

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


Index: llvm/lib/Analysis/BasicAliasAnalysis.cpp
===================================================================
--- llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -1394,7 +1394,8 @@
 
   if (!DecompGEP1.VarIndices.empty()) {
     APInt Modulo(MaxPointerSize, 0);
-    bool AllPositive = true;
+    bool AllNonNegative = GEP1BaseOffset.isNonNegative();
+    bool AllNonPositive = GEP1BaseOffset.isNonPositive();
     for (unsigned i = 0, e = DecompGEP1.VarIndices.size(); i != e; ++i) {
 
       // Try to distinguish something like &A[i][1] against &A[42][0].
@@ -1403,7 +1404,7 @@
       // be ^'ing Modulo with itself later.
       Modulo |= DecompGEP1.VarIndices[i].Scale;
 
-      if (AllPositive) {
+      if (AllNonNegative || AllNonPositive) {
         // If the Value could change between cycles, then any reasoning about
         // the Value this cycle may not hold in the next cycle. We'll just
         // give up if we can't determine conditions that hold for every cycle:
@@ -1420,12 +1421,11 @@
         SignKnownZero |= IsZExt;
         SignKnownOne &= !IsZExt;
 
-        // If the variable begins with a zero then we know it's
-        // positive, regardless of whether the value is signed or
-        // unsigned.
         APInt Scale = DecompGEP1.VarIndices[i].Scale;
-        AllPositive =
-            (SignKnownZero && Scale.sge(0)) || (SignKnownOne && Scale.slt(0));
+        AllNonNegative &= (SignKnownZero && Scale.isNonNegative()) ||
+                          (SignKnownOne && Scale.isNonPositive());
+        AllNonPositive &= (SignKnownZero && Scale.isNonPositive()) ||
+                          (SignKnownOne && Scale.isNonNegative());
       }
     }
 
@@ -1440,13 +1440,16 @@
         (Modulo - ModOffset).uge(V1Size.getValue()))
       return NoAlias;
 
-    // If we know all the variables are positive, then GEP1 >= GEP1BasePtr.
-    // If GEP1BasePtr > V2 (GEP1BaseOffset > 0) then we know the pointers
-    // don't alias if V2Size can fit in the gap between V2 and GEP1BasePtr.
-    if (AllPositive && GEP1BaseOffset.sgt(0) &&
-        V2Size != LocationSize::unknown() &&
+    // If we know all the variables are non-negative, then the total offset is
+    // also non-negative and >= GEP1BaseOffset. If GEP1BaseOffset >= V2Size,
+    // then the pointers can't alias. An analogous case holds for the
+    // non-positive case as well.
+    if (AllNonNegative && V2Size != LocationSize::unknown() &&
         GEP1BaseOffset.uge(V2Size.getValue()))
       return NoAlias;
+    if (AllNonPositive && V2Size != LocationSize::unknown() &&
+        (-GEP1BaseOffset).uge(V2Size.getValue()))
+      return NoAlias;
 
     if (constantOffsetHeuristic(DecompGEP1.VarIndices, V1Size, V2Size,
                                 GEP1BaseOffset, &AC, DT))
Index: llvm/lib/Analysis/AliasAnalysisEvaluator.cpp
===================================================================
--- llvm/lib/Analysis/AliasAnalysisEvaluator.cpp
+++ llvm/lib/Analysis/AliasAnalysisEvaluator.cpp
@@ -170,6 +170,11 @@
         ++MustAliasCount;
         break;
       }
+
+      // We assume that alias(I1, I2) == alias(I2, I1) and only print one
+      // order. Make sure this assumption actually holds.
+      assert(AR == AA.alias(*I2, I2Size, *I1, I1Size) &&
+             "AA query not symmetric");
     }
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D91383.304942.patch
Type: text/x-patch
Size: 3371 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201112/8493b641/attachment.bin>