Make GlobalsAA ignore dead constant expressions.

[efriedma]

Summary

Slightly improves the precision of GlobalsAA in certain situations, and makes the behavior of optimization passes more predictable.

[mkuper]

lib/Analysis/GlobalsModRef.cpp

277

Don't know enough about GlobalsAA to approve this anyway, but as a matter of principle - is it acceptable for an analysis pass to modify the IR?

[hfinkel]

lib/Analysis/GlobalsModRef.cpp

277

No, they shouldn't.

[sanjoy]

Marking as requiring changes to get this out of my phabricator queue. I think @mkuper 's comment on not modifying IR from analyses is a blocker.

[efriedma]

Revised approach which doesn't modify the IR.

[efriedma]

Ping.

[sanjoy]

Random drop-by comment.

test/Analysis/GlobalsModRef/dead-uses.ll

38

There is no #0 in the file.

[efriedma]

Updated test.

[efriedma]

Ping.

[sanjoy]

lib/Analysis/GlobalsModRef.cpp

369

How about pulling this out of the loop? That is, do:

if (auto *C = dyn_cast<Constant>(V))
  if (!C->isConstantUsed())
    return false;

test/Analysis/GlobalsModRef/dead-uses.ll

12

Some more context here will be helpful -- are you trying to check if the load was hoisted out of the loop? If so, then a CHECK: entry: before the load will be nice (since there are two instances of br label %for.inc).

[efriedma]

lib/Analysis/GlobalsModRef.cpp

369

That wouldn't work... say V is the global @a (with live loads and stores which access it), and I is a constant expression ptrtoint @a to i32. We want to ignore I if it's dead; checking V is useless.

test/Analysis/GlobalsModRef/dead-uses.ll

12

Yes; I'll change this to make that more clear.

[efriedma]

Make test more clear.

[sanjoy]

I don't understand GlobalsModRef deeply, but this seems obvious so lgtm.

[chandlerc]

Thanks Sanjoy for doing the review. FWIW, I agree, LGTM.

[efriedma]

Merged in r283165.

lib/Analysis/GlobalsModRef.cpp

 /// GlobalValue's in the program.  If none of them have their "address taken"
 /// (really, their address passed to something nontrivial), record this fact,
 /// and record the functions that they are used directly in.
 void GlobalsAAResult::AnalyzeGlobals(Module &M) {
   SmallPtrSet<Function *, 32> TrackedFunctions;
   for (Function &F : M)
     if (F.hasLocalLinkage())
       if (!AnalyzeUsesOfPointer(&F)) {
         // Remember that we are tracking this global.
         NonAddressTakenGlobals.insert(&F);
         TrackedFunctions.insert(&F);
         Handles.emplace_front(*this, &F);
         Handles.front().I = Handles.begin();
         ++NumNonAddrTakenFunctions;
       }
 
   SmallPtrSet<Function *, 16> Readers, Writers;
if (LoadInst *LI = dyn_cast<LoadInst>(I)) {
             Writers->insert(CS->getParent()->getParent());
         } else {
           return true; // Argument of an unknown call.
         }
       }
     } else if (ICmpInst *ICI = dyn_cast<ICmpInst>(I)) {
       if (!isa<ConstantPointerNull>(ICI->getOperand(1)))
         return true; // Allow comparison against null.
+    } else if (Constant *C = dyn_cast<Constant>(I)) {
+      return C->isConstantUsed();
     } else {
       return true;
     }
   }
 
   return false;
 }
 

test/Analysis/GlobalsModRef/dead-uses.ll

+; RUN: opt < %s -instcombine -globals-aa -licm -S | FileCheck %s
+
+; Make sure -globals-aa ignores dead uses of globals.
+
+@a = internal global i32 0, align 4
+@c = common global i32 0, align 4
+
+; Function Attrs: nounwind
+define i32 @g() {
+; Make sure the load of @a is hoisted.
+; CHECK-LABEL: define i32 @g()
+; CHECK: entry:
+; CHECK-NEXT: load i32, i32* @a, align 4
+; CHECK-NEXT: br label %for.cond
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.inc, %entry
+  %i.0 = phi i32 [ 0, %entry ], [ %inc, %for.inc ]
+  %sum.0 = phi i32 [ 0, %entry ], [ %add, %for.inc ]
+  %cmp = icmp slt i32 %i.0, 1000
+  br i1 %cmp, label %for.body, label %for.end
+
+for.body:                                         ; preds = %for.cond
+  %0 = load i32, i32* @a, align 4
+  %add = add nsw i32 %sum.0, %0
+  call void @f()
+  br label %for.inc
+
+for.inc:                                          ; preds = %for.body
+  %inc = add nsw i32 %i.0, 1
+  br label %for.cond
+
+for.end:                                          ; preds = %for.cond
+  ret i32 %sum.0
+}
+
+; Function Attrs: nounwind
+define internal void @f() {
+entry:
+  %tobool = icmp ne i32 0, 0
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:                                          ; preds = %entry
+  store i32 ptrtoint (i32* @a to i32), i32* @c, align 4
+  br label %if.end
+
+if.end:                                           ; preds = %if.then, %entry
+  %0 = load i32, i32* @c, align 4
+  %inc = add nsw i32 %0, 1
+  store i32 %inc, i32* @c, align 4
+  ret void
+}
+