Fix LoopAccessAnalysis when potentially nullptr check are involved

[mehdi_amini]

Summary

GetUnderlyingObjects() can return "null" among its list of objects,
we don't want to deduce that two pointers can point to the same
memory in this case, so filter it out.

[anemet]

LGTM with the comments. Thanks for the fix!

lib/Analysis/LoopAccessAnalysis.cpp

748–750

No {}

test/Analysis/LoopAccessAnalysis/nullptr.ll

2

space after ;

Also a sentence of what this is trying to test would be useful.

[mehdi_amini]

Update to commited revision (r252149)

lib/Analysis/LoopAccessAnalysis.cpp

void RuntimePointerChecking::groupChecks(
   // dependence. Not grouping the checks for a[i] and a[i + 9000] allows
   // us to perform an accurate check in this case.
   //
   // The above case requires that we have an UnknownDependence between
   // accesses to the same underlying object. This cannot happen unless
   // ShouldRetryWithRuntimeCheck is set, and therefore UseDependencies
   // is also false. In this case we will use the fallback path and create
   // separate checking groups for all pointers.
- 
   // If we don't have the dependency partitions, construct a new
   // checking pointer group for each pointer. This is also required
   // for correctness, because in this case we can have checking between
   // pointers to the same underlying object.
   if (!UseDependencies) {
     for (unsigned I = 0; I < Pointers.size(); ++I)
       CheckingGroups.push_back(CheckingPtrGroup(I, *this));
     return;
for (int SetIteration = 0; SetIteration < 2; ++SetIteration) {
           // Create sets of pointers connected by a shared alias set and
           // underlying object.
           typedef SmallVector<Value *, 16> ValueVector;
           ValueVector TempObjects;
 
           GetUnderlyingObjects(Ptr, TempObjects, DL, LI);
           DEBUG(dbgs() << "Underlying objects for pointer " << *Ptr << "\n");
           for (Value *UnderlyingObj : TempObjects) {
+            // nullptr never alias, don't join sets for pointer that have "null"
+            // in their UnderlyingObjects list.
+            if (isa<ConstantPointerNull>(UnderlyingObj))
+              continue;
+
             UnderlyingObjToAccessMap::iterator Prev =
                 ObjToLastAccess.find(UnderlyingObj);
             if (Prev != ObjToLastAccess.end())
               DepCands.unionSets(Access, Prev->second);
 
             ObjToLastAccess[UnderlyingObj] = Access;
             DEBUG(dbgs() << "  " << *UnderlyingObj << "\n");
           }

test/Analysis/LoopAccessAnalysis/nullptr.ll

+; RUN: opt -loop-accesses -analyze %s  | FileCheck %s
+
+; Test that the loop accesses are proven safe in this case.
+; The analyzer uses to be confused by the "diamond" because GetUnderlyingObjects
+; is saying that the two pointers can both points to null. The loop analyzer
+; needs to ignore null in the results returned by GetUnderlyingObjects.
+
+; CHECK: Memory dependences are safe with run-time checks
+
+
+; ModuleID = 'bugpoint-reduced-simplified.bc'
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+; Function Attrs: ssp uwtable
+define void @foo(i1 %cond, i32* %ptr1, i32* %ptr2)  {
+  br i1 %cond, label %.preheader, label %diamond
+
+diamond:  ; preds = %.noexc.i.i
+  br label %.preheader
+
+.preheader:                                     ; preds = %diamond, %0
+  %ptr1_or_null = phi i32* [ null, %0 ], [ %ptr1, %diamond ]
+  %ptr2_or_null = phi i32* [ null, %0 ], [ %ptr2, %diamond ]
+  br label %.lr.ph
+
+.lr.ph:                                           ; preds = %.lr.ph, %.preheader
+  %indvars.iv = phi i64 [ %indvars.iv.next, %.lr.ph ], [ 10, %.preheader ]
+  %indvars.iv.next = add nsw i64 %indvars.iv, -1
+  %tmp4 = getelementptr inbounds i32, i32* %ptr2_or_null, i64 %indvars.iv.next
+  %tmp5 = load i32, i32* %tmp4, align 4
+  %tmp6 = getelementptr inbounds i32, i32* %ptr1_or_null, i64 %indvars.iv.next
+  store i32 undef, i32* %tmp6, align 4
+  br i1 false, label %.lr.ph, label %.end
+
+.end:
+  ret void
+}