From 1ba99eaf70951a4aa062688577047d8a23bfe54c Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Sat, 18 Dec 2021 09:11:33 +0100 Subject: [PATCH] Revert "[DSE] Remove calls with known writes to dead memory" This reverts commit a8a51fe55649f5e07f9f2973507dc20bc4e40765. This breaks the strncpy-overflow.cpp test case. --- llvm/lib/Analysis/MemoryLocation.cpp | 39 +--------------------- .../InstCombine/InstructionCombining.cpp | 33 ++++++++++++------ .../DeadStoreElimination/trivial-dse-calls.ll | 18 +++++++++- 3 files changed, 40 insertions(+), 50 deletions(-) diff --git a/llvm/lib/Analysis/MemoryLocation.cpp b/llvm/lib/Analysis/MemoryLocation.cpp index 6365828..c74b625 100644 --- a/llvm/lib/Analysis/MemoryLocation.cpp +++ b/llvm/lib/Analysis/MemoryLocation.cpp @@ -147,44 +147,7 @@ MemoryLocation::getForDest(const CallBase *CB, const TargetLibraryInfo &TLI) { } } - if (!CB->onlyAccessesArgMemory()) - return None; - - if (CB->hasOperandBundles()) - // TODO: remove implementation restriction - return None; - - Value *UsedV = nullptr; - Optional UsedIdx; - for (unsigned i = 0; i < CB->arg_size(); i++) { - if (!CB->getArgOperand(i)->getType()->isPointerTy()) - continue; - if (!CB->doesNotCapture(i)) - // capture would allow the address to be read back in an untracked manner - return None; - if (CB->onlyReadsMemory(i)) - continue; - if (!UsedV) { - // First potentially writing parameter - UsedV = CB->getArgOperand(i); - UsedIdx = i; - continue; - } - UsedIdx = None; - if (UsedV != CB->getArgOperand(i)) - // Can't describe writing to two distinct locations. - // TODO: This results in an inprecision when two values derived from the - // same object are passed as arguments to the same function. - return None; - } - if (!UsedV) - // We don't currently have a way to represent a "does not write" result - // and thus have to be conservative and return unknown. - return None; - - if (UsedIdx) - return getForArgument(CB, *UsedIdx, &TLI); - return MemoryLocation::getBeforeOrAfter(UsedV, CB->getAAMetadata()); + return None; } MemoryLocation MemoryLocation::getForArgument(const CallBase *Call, diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index eb5eadb..0657d29 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -2562,24 +2562,35 @@ static bool isNeverEqualToUnescapedAlloc(Value *V, const TargetLibraryInfo &TLI, /// Given a call CB which uses an address UsedV, return true if we can prove the /// call's only possible effect is storing to V. -static bool isRemovableWrite(CallBase &CB, Value *UsedV, - const TargetLibraryInfo &TLI) { +static bool isRemovableWrite(CallBase &CB, Value *UsedV) { if (!CB.use_empty()) // TODO: add recursion if returned attribute is present return false; - if (CB.isTerminator()) - // TODO: remove implementation restriction + if (!CB.willReturn() || !CB.doesNotThrow() || !CB.onlyAccessesArgMemory() || + CB.isTerminator()) return false; - if (!CB.willReturn() || !CB.doesNotThrow()) + if (CB.hasOperandBundles()) return false; - // If the only possible side effect of the call is writing to the alloca, - // and the result isn't used, we can safely remove any reads implied by the - // call including those which might read the alloca itself. - Optional Dest = MemoryLocation::getForDest(&CB, TLI); - return Dest && Dest->Ptr == UsedV; + for (unsigned i = 0; i < CB.arg_size(); i++) { + if (!CB.getArgOperand(i)->getType()->isPointerTy()) + continue; + if (!CB.doesNotCapture(i)) + // capture would allow the address to be read back in an untracked manner + return false; + if (UsedV != CB.getArgOperand(i) && !CB.onlyReadsMemory(i)) + // A write to another memory location keeps the call live, and thus we + // must keep the alloca so that the call has somewhere to write to. + // TODO: This results in an inprecision when two values derived from the + // same alloca are passed as arguments to the same function. + return false; + // Note: Both reads from and writes to the alloca are fine. Since the + // result is unused nothing can observe the values read from the alloca + // without writing it to some other observable location (checked above). + } + return true; } static bool isAllocSiteRemovable(Instruction *AI, @@ -2649,7 +2660,7 @@ static bool isAllocSiteRemovable(Instruction *AI, } } - if (isRemovableWrite(*cast(I), PI, TLI)) { + if (isRemovableWrite(*cast(I), PI)) { Users.emplace_back(I); continue; } diff --git a/llvm/test/Transforms/DeadStoreElimination/trivial-dse-calls.ll b/llvm/test/Transforms/DeadStoreElimination/trivial-dse-calls.ll index 13fd0e2..caac984 100644 --- a/llvm/test/Transforms/DeadStoreElimination/trivial-dse-calls.ll +++ b/llvm/test/Transforms/DeadStoreElimination/trivial-dse-calls.ll @@ -11,6 +11,9 @@ declare void @f2(i8*, i8*) ; Basic case for DSEing a trivially dead writing call define void @test_dead() { ; CHECK-LABEL: @test_dead( +; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4 +; CHECK-NEXT: [[BITCAST:%.*]] = bitcast i32* [[A]] to i8* +; CHECK-NEXT: call void @f(i8* nocapture writeonly [[BITCAST]]) #[[ATTR1:[0-9]+]] ; CHECK-NEXT: ret void ; %a = alloca i32, align 4 @@ -25,6 +28,7 @@ define void @test_lifetime() { ; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4 ; CHECK-NEXT: [[BITCAST:%.*]] = bitcast i32* [[A]] to i8* ; CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 4, i8* [[BITCAST]]) +; CHECK-NEXT: call void @f(i8* nocapture writeonly [[BITCAST]]) #[[ATTR1]] ; CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 4, i8* [[BITCAST]]) ; CHECK-NEXT: ret void ; @@ -44,6 +48,7 @@ define void @test_lifetime2() { ; CHECK-NEXT: [[BITCAST:%.*]] = bitcast i32* [[A]] to i8* ; CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 4, i8* [[BITCAST]]) ; CHECK-NEXT: call void @unknown() +; CHECK-NEXT: call void @f(i8* nocapture writeonly [[BITCAST]]) #[[ATTR1]] ; CHECK-NEXT: call void @unknown() ; CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 4, i8* [[BITCAST]]) ; CHECK-NEXT: ret void @@ -62,6 +67,9 @@ define void @test_lifetime2() { ; itself since the write will be dropped. define void @test_dead_readwrite() { ; CHECK-LABEL: @test_dead_readwrite( +; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4 +; CHECK-NEXT: [[BITCAST:%.*]] = bitcast i32* [[A]] to i8* +; CHECK-NEXT: call void @f(i8* nocapture [[BITCAST]]) #[[ATTR1]] ; CHECK-NEXT: ret void ; %a = alloca i32, align 4 @@ -74,7 +82,7 @@ define i32 @test_neg_read_after() { ; CHECK-LABEL: @test_neg_read_after( ; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4 ; CHECK-NEXT: [[BITCAST:%.*]] = bitcast i32* [[A]] to i8* -; CHECK-NEXT: call void @f(i8* nocapture writeonly [[BITCAST]]) #[[ATTR1:[0-9]+]] +; CHECK-NEXT: call void @f(i8* nocapture writeonly [[BITCAST]]) #[[ATTR1]] ; CHECK-NEXT: [[RES:%.*]] = load i32, i32* [[A]], align 4 ; CHECK-NEXT: ret i32 [[RES]] ; @@ -195,6 +203,11 @@ define i32 @test_neg_captured_before() { ; Show that reading from unrelated memory is okay define void @test_unreleated_read() { ; CHECK-LABEL: @test_unreleated_read( +; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4 +; CHECK-NEXT: [[A2:%.*]] = alloca i32, align 4 +; CHECK-NEXT: [[BITCAST:%.*]] = bitcast i32* [[A]] to i8* +; CHECK-NEXT: [[BITCAST2:%.*]] = bitcast i32* [[A2]] to i8* +; CHECK-NEXT: call void @f2(i8* nocapture writeonly [[BITCAST]], i8* nocapture readonly [[BITCAST2]]) #[[ATTR1]] ; CHECK-NEXT: ret void ; %a = alloca i32, align 4 @@ -227,6 +240,9 @@ define void @test_neg_unreleated_capture() { ; itself since the write will be dropped. define void @test_self_read() { ; CHECK-LABEL: @test_self_read( +; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4 +; CHECK-NEXT: [[BITCAST:%.*]] = bitcast i32* [[A]] to i8* +; CHECK-NEXT: call void @f2(i8* nocapture writeonly [[BITCAST]], i8* nocapture readonly [[BITCAST]]) #[[ATTR1]] ; CHECK-NEXT: ret void ; %a = alloca i32, align 4 -- 2.7.4