[llvm-branch-commits] [llvm] bfda694 - [BasicAA] Fix a bug with relational reasoning across iterations

[Philip Reames via llvm-branch-commits]

Author: Philip Reames
Date: 2020-12-05T14:10:21-08:00
New Revision: bfda69416c6d0a76b40644b1b0cbc1cbca254a61

URL: https://github.com/llvm/llvm-project/commit/bfda69416c6d0a76b40644b1b0cbc1cbca254a61
DIFF: https://github.com/llvm/llvm-project/commit/bfda69416c6d0a76b40644b1b0cbc1cbca254a61.diff

LOG: [BasicAA] Fix a bug with relational reasoning across iterations

Due to the recursion through phis basicaa does, the code needs to be extremely careful not to reason about equality between values which might represent distinct iterations. I'm generally skeptical of the correctness of the whole scheme, but this particular patch fixes one particular instance which is demonstrateable incorrect.

Interestingly, this appears to be the second attempted fix for the same issue. The former fix is incomplete and doesn't address the actual issue.

Differential Revision: https://reviews.llvm.org/D92694

Added: 
    

Modified: 
    llvm/include/llvm/Analysis/BasicAliasAnalysis.h
    llvm/lib/Analysis/BasicAliasAnalysis.cpp
    llvm/test/Analysis/BasicAA/phi-aa.ll

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/Analysis/BasicAliasAnalysis.h b/llvm/include/llvm/Analysis/BasicAliasAnalysis.h
index d9a174951695..eedecd2a4381 100644
--- a/llvm/include/llvm/Analysis/BasicAliasAnalysis.h
+++ b/llvm/include/llvm/Analysis/BasicAliasAnalysis.h
@@ -202,6 +202,12 @@ class BasicAAResult : public AAResultBase<BasicAAResult> {
       const DecomposedGEP &DecompGEP, const DecomposedGEP &DecompObject,
       LocationSize ObjectAccessSize);
 
+  AliasResult aliasSameBasePointerGEPs(const GEPOperator *GEP1,
+                                       LocationSize MaybeV1Size,
+                                       const GEPOperator *GEP2,
+                                       LocationSize MaybeV2Size,
+                                       const DataLayout &DL);
+
   /// A Heuristic for aliasGEP that searches for a constant offset
   /// between the variables.
   ///

diff  --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index 2fb353eabb6e..5e611a9e193c 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -1032,11 +1032,11 @@ ModRefInfo BasicAAResult::getModRefInfo(const CallBase *Call1,
 
 /// Provide ad-hoc rules to disambiguate accesses through two GEP operators,
 /// both having the exact same pointer operand.
-static AliasResult aliasSameBasePointerGEPs(const GEPOperator *GEP1,
-                                            LocationSize MaybeV1Size,
-                                            const GEPOperator *GEP2,
-                                            LocationSize MaybeV2Size,
-                                            const DataLayout &DL) {
+AliasResult BasicAAResult::aliasSameBasePointerGEPs(const GEPOperator *GEP1,
+                                                    LocationSize MaybeV1Size,
+                                                    const GEPOperator *GEP2,
+                                                    LocationSize MaybeV2Size,
+                                                    const DataLayout &DL) {
   assert(GEP1->getPointerOperand()->stripPointerCastsAndInvariantGroups() ==
              GEP2->getPointerOperand()->stripPointerCastsAndInvariantGroups() &&
          GEP1->getPointerOperandType() == GEP2->getPointerOperandType() &&
@@ -1126,24 +1126,12 @@ static AliasResult aliasSameBasePointerGEPs(const GEPOperator *GEP1,
     if (C1 && C2)
       return NoAlias;
     {
+      // If we're not potentially reasoning about values from 
diff erent
+      // iterations, see if we can prove them inequal.
       Value *GEP1LastIdx = GEP1->getOperand(GEP1->getNumOperands() - 1);
       Value *GEP2LastIdx = GEP2->getOperand(GEP2->getNumOperands() - 1);
-      if (isa<PHINode>(GEP1LastIdx) || isa<PHINode>(GEP2LastIdx)) {
-        // If one of the indices is a PHI node, be safe and only use
-        // computeKnownBits so we don't make any assumptions about the
-        // relationships between the two indices. This is important if we're
-        // asking about values from 
diff erent loop iterations. See PR32314.
-        // TODO: We may be able to change the check so we only do this when
-        // we definitely looked through a PHINode.
-        if (GEP1LastIdx != GEP2LastIdx &&
-            GEP1LastIdx->getType() == GEP2LastIdx->getType()) {
-          KnownBits Known1 = computeKnownBits(GEP1LastIdx, DL);
-          KnownBits Known2 = computeKnownBits(GEP2LastIdx, DL);
-          if (Known1.Zero.intersects(Known2.One) ||
-              Known1.One.intersects(Known2.Zero))
-            return NoAlias;
-        }
-      } else if (isKnownNonEqual(GEP1LastIdx, GEP2LastIdx, DL))
+      if (VisitedPhiBBs.empty() &&
+          isKnownNonEqual(GEP1LastIdx, GEP2LastIdx, DL))
         return NoAlias;
     }
   }

diff  --git a/llvm/test/Analysis/BasicAA/phi-aa.ll b/llvm/test/Analysis/BasicAA/phi-aa.ll
index f5492e6fb00f..bf5915f1e0fe 100644
--- a/llvm/test/Analysis/BasicAA/phi-aa.ll
+++ b/llvm/test/Analysis/BasicAA/phi-aa.ll
@@ -173,3 +173,27 @@ exit:
 }
 
 declare void @llvm.memset.p0i8.i32(i8*, i8, i32, i1)
+
+; CHECK-LABEL: unsound_inequality
+; CHECK: MayAlias:  i32* %arrayidx13, i32* %phi
+; CHECK: MayAlias:  i32* %arrayidx5, i32* %phi
+; CHECK: NoAlias:   i32* %arrayidx13, i32* %arrayidx5
+
+; When recursively reasoning about phis, we can't use predicates between
+; two values as we might be comparing the two from 
diff erent iterations.
+define i32 @unsound_inequality(i32* %jj7, i32* %j) {
+entry:
+  %oa5 = alloca [100 x i32], align 16
+  br label %for.body
+
+for.body:                                         ; preds = %for.body, %entry
+  %phi = phi i32* [ %arrayidx13, %for.body ], [ %j, %entry ]
+  %idx = load i32, i32* %jj7, align 4
+  %arrayidx5 = getelementptr inbounds [100 x i32], [100 x i32]* %oa5, i64 0, i32 %idx
+  store i32 0, i32* %arrayidx5, align 4
+  store i32 0, i32* %phi, align 4
+  %notequal = add i32 %idx, 1
+  %arrayidx13 = getelementptr inbounds [100 x i32], [100 x i32]* %oa5, i64 0, i32 %notequal
+  store i32 0, i32* %arrayidx13, align 4
+  br label %for.body
+}