[BasicAA] Fix handling of invariant group launders

[amharc]

Summary

A recent patch (rL331587) to Capture Tracking taught it that the launder_invariant_group intrinsic captures its argument only by returning it. Unfortunately, BasicAA still considered every call instruction as a possible escape source and hence concluded that the result of a launder_invariant_group call cannot alias any local non-escaping value. This led to bug 37458.

This patch updates the relevant check for escape sources in BasicAA.

[Prazek]

I went throught it and it looks good to me, I didn't expect that fixing CaptureTracking can lead Alias Analisis to return NoAlias instead of MustAlias. It would be nice if the code would check for such assumptions, but I have no ideas how it could be implemented.

[hfinkel]

We should also have a direct AA test (something like test/Analysis/BasicAA/cs-cs.ll).

Also, let's not repeat this problem in the future. Please add a comment both here and in CaptureTracking to remind people that intrinsics added to once place must be added to the other place.

llvm/lib/Analysis/BasicAliasAnalysis.cpp

138 ↗

(On Diff #146901)

Don't need the { } here.

[amharc]

Added a direct AA test and cautionary comments in both BasicAA and CaptureTracking, removed unnecessary { }.

[amharc]

Removed unnecessary { }.

[hfinkel]

LGTM

llvm/trunk/lib/Analysis/BasicAliasAnalysis.cpp

if (A->hasByValAttr() || A->hasNoAliasAttr())
       return !PointerMayBeCaptured(V, false, /*StoreCaptures=*/true);
 
   return false;
 }
 
 /// Returns true if the pointer is one which would have been considered an
 /// escape by isNonEscapingLocalObject.
 static bool isEscapeSource(const Value *V) {
-  if (isa<CallInst>(V) || isa<InvokeInst>(V) || isa<Argument>(V))
+  if (auto CS = ImmutableCallSite(V)) {
+    // launder_invariant_group captures its argument only by returning it,
+    // so it might not be considered an escape by isNonEscapingLocalObject.
+    // Note that adding similar special cases for intrinsics in CaptureTracking
+    // requires handling them here too.
+    if (CS.getIntrinsicID() == Intrinsic::launder_invariant_group)
+      return false;
+
+    return true;
+  }
+
+  if (isa<Argument>(V))
     return true;
 
   // The load case works because isNonEscapingLocalObject considers all
   // stores to be escapes (it passes true for the StoreCaptures argument
   // to PointerMayBeCaptured).
   if (isa<LoadInst>(V))
     return true;
 

llvm/trunk/lib/Analysis/CaptureTracking.cpp

case Instruction::Invoke: {
       // Not captured if the callee is readonly, doesn't return a copy through
       // its return value and doesn't unwind (a readonly function can leak bits
       // by throwing an exception or not depending on the input value).
       if (CS.onlyReadsMemory() && CS.doesNotThrow() && I->getType()->isVoidTy())
         break;
 
       // launder.invariant.group only captures pointer by returning it,
       // so the pointer wasn't captured if returned pointer is not captured.
+      // Note that adding similar special cases for intrinsics requires handling
+      // them in 'isEscapeSource' in BasicAA.
       if (CS.getIntrinsicID() == Intrinsic::launder_invariant_group) {
         AddUses(I);
         break;
       }
 
       // Volatile operations effectively capture the memory location that they
       // load and store to.
       if (auto *MI = dyn_cast<MemIntrinsic>(I))

llvm/trunk/test/Analysis/BasicAA/invariant_group.ll

+; RUN: opt < %s -basicaa -aa-eval -print-all-alias-modref-info -disable-output 2>&1 | FileCheck %s
+; RUN: opt < %s -basicaa -gvn -S | FileCheck -check-prefix=CHECK-GVN %s
+
+; The input *.ll had been adapted from bug 37458:
+;
+; struct A { virtual void f(); int n; };
+;
+; int h() {
+;     A a;
+;     a.n = 42;
+;     return __builtin_launder(&a)->n;
+; }
+
+%struct.A = type <{ i8*, i8 }>
+
+; CHECK: testLaunderInvariantGroupIsNotEscapeSource
+; CHECK-GVN: testLaunderInvariantGroupIsNotEscapeSource
+define i8 @testLaunderInvariantGroupIsNotEscapeSource() {
+; CHECK-DAG: MustAlias: %struct.A* %a, i8* %a.bitcast
+; CHECK-DAG: PartialAlias: %struct.A* %a, i8* %n
+; CHECK-DAG: NoAlias: i8* %a.bitcast, i8* %n
+; CHECK-DAG: MustAlias: %struct.A* %a, i8* %a.laundered
+; CHECK-DAG: MustAlias: i8* %a.bitcast, i8* %a.laundered
+; CHECK-DAG: NoAlias: i8* %a.laundered, i8* %n
+; CHECK-DAG: PartialAlias: %struct.A* %a, i8* %n.laundered
+; CHECK-DAG: NoAlias: i8* %a.bitcast, i8* %n.laundered
+; CHECK-DAG: MustAlias: i8* %n, i8* %n.laundered
+; CHECK-DAG: NoAlias: i8* %a.laundered, i8* %n.laundered
+; CHECK-DAG: NoModRef: Ptr: %struct.A* %a <-> %a.laundered = call i8* @llvm.launder.invariant.group.p0i8(i8* nonnull %a.bitcast)
+; CHECK-DAG: NoModRef: Ptr: i8* %a.bitcast <-> %a.laundered = call i8* @llvm.launder.invariant.group.p0i8(i8* nonnull %a.bitcast)
+; CHECK-DAG: NoModRef: Ptr: i8* %n <-> %a.laundered = call i8* @llvm.launder.invariant.group.p0i8(i8* nonnull %a.bitcast)
+; CHECK-DAG: NoModRef: Ptr: i8* %a.laundered <-> %a.laundered = call i8* @llvm.launder.invariant.group.p0i8(i8* nonnull %a.bitcast)
+; CHECK-DAG: NoModRef: Ptr: i8* %n.laundered <-> %a.laundered = call i8* @llvm.launder.invariant.group.p0i8(i8* nonnull %a.bitcast)
+
+entry:
+  %a = alloca %struct.A, align 8
+  %a.bitcast = bitcast %struct.A* %a to i8*
+  %n = getelementptr inbounds %struct.A, %struct.A* %a, i64 0, i32 1
+  store i8 42, i8* %n
+  %a.laundered = call i8* @llvm.launder.invariant.group.p0i8(i8* nonnull %a.bitcast)
+  %n.laundered = getelementptr inbounds i8, i8* %a.laundered, i64 8
+  %v = load i8, i8* %n.laundered
+; make sure that the load from %n.laundered to %v aliases the store of 42 to %n
+; CHECK-GVN: ret i8 42
+  ret i8 %v
+}
+
+declare i8* @llvm.launder.invariant.group.p0i8(i8*)