<div dir="ltr">Hey,<div><br></div><div>Can this be merged into the 7.0 branch, please? It fixes miscompiles, and we've had 0 issues with it for the few months that it's been patched into Android's toolchain</div><div><br></div><div>Thank you very much!</div><div>George</div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Aug 9, 2018 at 10:15 PM George Burgess IV via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: gbiv<br>
Date: Thu Aug 9 22:14:43 2018<br>
New Revision: 339411<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=339411&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=339411&view=rev</a><br>
Log:<br>
[MemorySSA] "Fix" lifetime intrinsic handling<br>
<br>
MemorySSA currently creates MemoryAccesses for lifetime intrinsics, and<br>
sometimes treats them as clobbers. This may/may not be the best way<br>
forward, but while we're doing it, we should consider<br>
MayAlias/PartialAlias to be clobbers.<br>
<br>
The ideal fix here is probably to remove all of this reasoning about<br>
lifetimes from MemorySSA + put it into the passes that need to care. But<br>
that's a wayyy broader fix that needs some consensus, and we have<br>
miscompiles + a release branch today, and this should solve the<br>
miscompiles just as well.<br>
<br>
differential revision is D43269. Landing without an explicit LGTM (and<br>
without using the special please-autoclose-this syntax) so we can still<br>
use that revision as a place to decide what the right fix here is.<br>
<br>
Modified:<br>
llvm/trunk/lib/Analysis/MemorySSA.cpp<br>
llvm/trunk/test/Transforms/EarlyCSE/memoryssa.ll<br>
llvm/trunk/unittests/Analysis/MemorySSA.cpp<br>
<br>
Modified: llvm/trunk/lib/Analysis/MemorySSA.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemorySSA.cpp?rev=339411&r1=339410&r2=339411&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemorySSA.cpp?rev=339411&r1=339410&r2=339411&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Analysis/MemorySSA.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/MemorySSA.cpp Thu Aug 9 22:14:43 2018<br>
@@ -258,13 +258,18 @@ static ClobberAlias instructionClobbersQ<br>
<br>
if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(DefInst)) {<br>
// These intrinsics will show up as affecting memory, but they are just<br>
- // markers.<br>
+ // markers, mostly.<br>
+ //<br>
+ // FIXME: We probably don't actually want MemorySSA to model these at all<br>
+ // (including creating MemoryAccesses for them): we just end up inventing<br>
+ // clobbers where they don't really exist at all. Please see D43269 for<br>
+ // context.<br>
switch (II->getIntrinsicID()) {<br>
case Intrinsic::lifetime_start:<br>
if (UseCS)<br>
return {false, NoAlias};<br>
AR = AA.alias(MemoryLocation(II->getArgOperand(1)), UseLoc);<br>
- return {AR == MustAlias, AR};<br>
+ return {AR != NoAlias, AR};<br>
case Intrinsic::lifetime_end:<br>
case Intrinsic::invariant_start:<br>
case Intrinsic::invariant_end:<br>
<br>
Modified: llvm/trunk/test/Transforms/EarlyCSE/memoryssa.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/EarlyCSE/memoryssa.ll?rev=339411&r1=339410&r2=339411&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/EarlyCSE/memoryssa.ll?rev=339411&r1=339410&r2=339411&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/EarlyCSE/memoryssa.ll (original)<br>
+++ llvm/trunk/test/Transforms/EarlyCSE/memoryssa.ll Thu Aug 9 22:14:43 2018<br>
@@ -106,3 +106,45 @@ end:<br>
store i32 %v2, i32* @G3<br>
ret void<br>
}<br>
+<br>
+;; Check that we respect lifetime.start/lifetime.end intrinsics when deleting<br>
+;; stores that, without the lifetime calls, would be writebacks.<br>
+; CHECK-LABEL: @test_writeback_lifetimes(<br>
+; CHECK-NOMEMSSA-LABEL: @test_writeback_lifetimes(<br>
+define void @test_writeback_lifetimes(i32* %p) {<br>
+entry:<br>
+ %q = getelementptr i32, i32* %p, i64 1<br>
+ %pv = load i32, i32* %p<br>
+ %qv = load i32, i32* %q<br>
+ call void @llvm.lifetime.end.p0i8(i64 8, i32* %p)<br>
+ call void @llvm.lifetime.start.p0i8(i64 8, i32* %p)<br>
+ ; CHECK: store i32 %pv<br>
+ ; CHECK-NOMEMSSA-LABEL: store i32 %pv<br>
+ store i32 %pv, i32* %p<br>
+ ; CHECK: store i32 %qv, i32* %q<br>
+ ; CHECK-NOMEMSSA-LABEL: store i32 %qv, i32* %q<br>
+ store i32 %qv, i32* %q<br>
+ ret void<br>
+}<br>
+<br>
+;; Check that we respect lifetime.start/lifetime.end intrinsics when deleting<br>
+;; stores that, without the lifetime calls, would be writebacks.<br>
+; CHECK-LABEL: @test_writeback_lifetimes_multi_arg(<br>
+; CHECK-NOMEMSSA-LABEL: @test_writeback_lifetimes_multi_arg(<br>
+define void @test_writeback_lifetimes_multi_arg(i32* %p, i32* %q) {<br>
+entry:<br>
+ %pv = load i32, i32* %p<br>
+ %qv = load i32, i32* %q<br>
+ call void @llvm.lifetime.end.p0i8(i64 8, i32* %p)<br>
+ call void @llvm.lifetime.start.p0i8(i64 8, i32* %p)<br>
+ ; CHECK: store i32 %pv<br>
+ ; CHECK-NOMEMSSA-LABEL: store i32 %pv<br>
+ store i32 %pv, i32* %p<br>
+ ; CHECK: store i32 %qv, i32* %q<br>
+ ; CHECK-NOMEMSSA-LABEL: store i32 %qv, i32* %q<br>
+ store i32 %qv, i32* %q<br>
+ ret void<br>
+}<br>
+<br>
+declare void @llvm.lifetime.end.p0i8(i64, i32*)<br>
+declare void @llvm.lifetime.start.p0i8(i64, i32*)<br>
<br>
Modified: llvm/trunk/unittests/Analysis/MemorySSA.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/MemorySSA.cpp?rev=339411&r1=339410&r2=339411&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/MemorySSA.cpp?rev=339411&r1=339410&r2=339411&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Analysis/MemorySSA.cpp (original)<br>
+++ llvm/trunk/unittests/Analysis/MemorySSA.cpp Thu Aug 9 22:14:43 2018<br>
@@ -1199,3 +1199,69 @@ TEST_F(MemorySSATest, TestStoreMayAlias)<br>
++I;<br>
}<br>
}<br>
+<br>
+TEST_F(MemorySSATest, LifetimeMarkersAreClobbers) {<br>
+ // Example code:<br>
+ // define void @a(i8* %foo) {<br>
+ // %bar = getelementptr i8, i8* %foo, i64 1<br>
+ // store i8 0, i8* %foo<br>
+ // store i8 0, i8* %bar<br>
+ // call void @llvm.lifetime.end.p0i8(i64 8, i32* %p)<br>
+ // call void @llvm.lifetime.start.p0i8(i64 8, i32* %p)<br>
+ // store i8 0, i8* %foo<br>
+ // store i8 0, i8* %bar<br>
+ // ret void<br>
+ // }<br>
+ //<br>
+ // Patterns like this are possible after inlining; the stores to %foo and %bar<br>
+ // should both be clobbered by the lifetime.start call if they're dominated by<br>
+ // it.<br>
+<br>
+ IRBuilder<> B(C);<br>
+ F = Function::Create(<br>
+ FunctionType::get(B.getVoidTy(), {B.getInt8PtrTy()}, false),<br>
+ GlobalValue::ExternalLinkage, "F", &M);<br>
+<br>
+ // Make blocks<br>
+ BasicBlock *Entry = BasicBlock::Create(C, "entry", F);<br>
+<br>
+ B.SetInsertPoint(Entry);<br>
+ Value *Foo = &*F->arg_begin();<br>
+<br>
+ Value *Bar = B.CreateGEP(Foo, B.getInt64(1), "bar");<br>
+<br>
+ B.CreateStore(B.getInt8(0), Foo);<br>
+ B.CreateStore(B.getInt8(0), Bar);<br>
+<br>
+ auto GetLifetimeIntrinsic = [&](Intrinsic::ID ID) {<br>
+ return Intrinsic::getDeclaration(&M, ID, {Foo->getType()});<br>
+ };<br>
+<br>
+ B.CreateCall(GetLifetimeIntrinsic(Intrinsic::lifetime_end),<br>
+ {B.getInt64(2), Foo});<br>
+ Instruction *LifetimeStart = B.CreateCall(<br>
+ GetLifetimeIntrinsic(Intrinsic::lifetime_start), {B.getInt64(2), Foo});<br>
+<br>
+ Instruction *FooStore = B.CreateStore(B.getInt8(0), Foo);<br>
+ Instruction *BarStore = B.CreateStore(B.getInt8(0), Bar);<br>
+<br>
+ setupAnalyses();<br>
+ MemorySSA &MSSA = *Analyses->MSSA;<br>
+<br>
+ MemoryAccess *LifetimeStartAccess = MSSA.getMemoryAccess(LifetimeStart);<br>
+ ASSERT_NE(LifetimeStartAccess, nullptr);<br>
+<br>
+ MemoryAccess *FooAccess = MSSA.getMemoryAccess(FooStore);<br>
+ ASSERT_NE(FooAccess, nullptr);<br>
+<br>
+ MemoryAccess *BarAccess = MSSA.getMemoryAccess(BarStore);<br>
+ ASSERT_NE(BarAccess, nullptr);<br>
+<br>
+ MemoryAccess *FooClobber =<br>
+ MSSA.getWalker()->getClobberingMemoryAccess(FooAccess);<br>
+ EXPECT_EQ(FooClobber, LifetimeStartAccess);<br>
+<br>
+ MemoryAccess *BarClobber =<br>
+ MSSA.getWalker()->getClobberingMemoryAccess(BarAccess);<br>
+ EXPECT_EQ(BarClobber, LifetimeStartAccess);<br>
+}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>