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