From: Chris Lattner Date: Mon, 6 Dec 2010 01:48:06 +0000 (+0000) Subject: Fix PR8728, a miscompilation I recently introduced. When optimizing X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;h=cc10244d7725f191bdc91cd62befff0c97257c7b;p=oota-llvm.git Fix PR8728, a miscompilation I recently introduced. When optimizing memcpy's like: memcpy(A, B) memcpy(A, C) we cannot delete the first memcpy as dead if A and C might be aliases. If so, we actually get: memcpy(A, B) memcpy(A, A) which is not correct to transform into: memcpy(A, A) This patch was heavily influenced by Jakub Staszak's patch in PR8728, thanks Jakub! git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@120974 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/llvm/Analysis/AliasAnalysis.h b/include/llvm/Analysis/AliasAnalysis.h index 7d5c5fc99b6..167371b6b28 100644 --- a/include/llvm/Analysis/AliasAnalysis.h +++ b/include/llvm/Analysis/AliasAnalysis.h @@ -188,6 +188,11 @@ public: return alias(LocA, LocB) == MustAlias; } + /// isMustAlias - A convenience wrapper. + bool isMustAlias(const Value *V1, const Value *V2) { + return alias(V1, 1, V2, 1) == MustAlias; + } + /// pointsToConstantMemory - If the specified memory location is /// known to be constant, return true. If OrLocal is true and the /// specified memory location is known to be "local" (derived from diff --git a/lib/Transforms/Scalar/DeadStoreElimination.cpp b/lib/Transforms/Scalar/DeadStoreElimination.cpp index f6b2999c294..ae5e727899b 100644 --- a/lib/Transforms/Scalar/DeadStoreElimination.cpp +++ b/lib/Transforms/Scalar/DeadStoreElimination.cpp @@ -198,6 +198,20 @@ getLocForWrite(Instruction *Inst, AliasAnalysis &AA) { } } +/// getLocForRead - Return the location read by the specified "hasMemoryWrite" +/// instruction if any. +static AliasAnalysis::Location +getLocForRead(Instruction *Inst, AliasAnalysis &AA) { + assert(hasMemoryWrite(Inst) && "Unknown instruction case"); + + // The only instructions that both read and write are the mem transfer + // instructions (memcpy/memmove). + if (MemTransferInst *MTI = dyn_cast(Inst)) + return AA.getLocationForSource(MTI); + return AliasAnalysis::Location(); +} + + /// isRemovable - If the value of this instruction and the memory it writes to /// is unused, may we delete this instruction? static bool isRemovable(Instruction *I) { @@ -347,6 +361,48 @@ static bool isCompleteOverwrite(const AliasAnalysis::Location &Later, return true; } +/// isPossibleSelfRead - If 'Inst' might be a self read (i.e. a noop copy of a +/// memory region into an identical pointer) then it doesn't actually make its +/// input dead in the traditional sense. Consider this case: +/// +/// memcpy(A <- B) +/// memcpy(A <- A) +/// +/// In this case, the second store to A does not make the first store to A dead. +/// The usual situation isn't an explicit A<-A store like this (which can be +/// trivially removed) but a case where two pointers may alias. +/// +/// This function detects when it is unsafe to remove a dependent instruction +/// because the DSE inducing instruction may be a self-read. +static bool isPossibleSelfRead(Instruction *Inst, + const AliasAnalysis::Location &InstStoreLoc, + Instruction *DepWrite, AliasAnalysis &AA) { + // Self reads can only happen for instructions that read memory. Get the + // location read. + AliasAnalysis::Location InstReadLoc = getLocForRead(Inst, AA); + if (InstReadLoc.Ptr == 0) return false; // Not a reading instruction. + + // If the read and written loc obviously don't alias, it isn't a read. + if (AA.isNoAlias(InstReadLoc, InstStoreLoc)) return false; + + // Okay, 'Inst' may copy over itself. However, we can still remove a the + // DepWrite instruction if we can prove that it reads from the same location + // as Inst. This handles useful cases like: + // memcpy(A <- B) + // memcpy(A <- B) + // Here we don't know if A/B may alias, but we do know that B/B are must + // aliases, so removing the first memcpy is safe (assuming it writes <= # + // bytes as the second one. + AliasAnalysis::Location DepReadLoc = getLocForRead(DepWrite, AA); + + if (DepReadLoc.Ptr && AA.isMustAlias(InstReadLoc.Ptr, DepReadLoc.Ptr)) + return false; + + // If DepWrite doesn't read memory or if we can't prove it is a must alias, + // then it can't be considered dead. + return true; +} + //===----------------------------------------------------------------------===// // DSE Pass @@ -423,9 +479,11 @@ bool DSE::runOnBasicBlock(BasicBlock &BB) { if (DepLoc.Ptr == 0) break; - // If we find a removable write that is completely obliterated by the - // store to 'Loc' then we can remove it. - if (isRemovable(DepWrite) && isCompleteOverwrite(Loc, DepLoc, *AA)) { + // If we find a write that is a) removable (i.e., non-volatile), b) is + // completely obliterated by the store to 'Loc', and c) which we know that + // 'Inst' doesn't load from, then we can remove it. + if (isRemovable(DepWrite) && isCompleteOverwrite(Loc, DepLoc, *AA) && + !isPossibleSelfRead(Inst, Loc, DepWrite, *AA)) { // Delete the store and now-dead instructions that feed it. DeleteDeadInstruction(DepWrite, *MD); ++NumFastStores; @@ -480,8 +538,7 @@ bool DSE::HandleFree(CallInst *F) { getStoredPointerOperand(Dependency)->getUnderlyingObject(); // Check for aliasing. - if (AA->alias(F->getArgOperand(0), 1, DepPointer, 1) != - AliasAnalysis::MustAlias) + if (!AA->isMustAlias(F->getArgOperand(0), DepPointer)) return false; // DCE instructions only used to calculate that store diff --git a/test/Transforms/DeadStoreElimination/simple.ll b/test/Transforms/DeadStoreElimination/simple.ll index 203cf37193b..a61eac9729e 100644 --- a/test/Transforms/DeadStoreElimination/simple.ll +++ b/test/Transforms/DeadStoreElimination/simple.ll @@ -203,7 +203,7 @@ define void @test16(i8* %P, i8* %Q) nounwind ssp { } ;; Overwrite of memset by memcpy. -define void @test17(i8* %P, i8* %Q) nounwind ssp { +define void @test17(i8* %P, i8* noalias %Q) nounwind ssp { tail call void @llvm.memset.i64(i8* %P, i8 42, i64 8, i32 1) tail call void @llvm.memcpy.i64(i8* %P, i8* %Q, i64 12, i32 1) ret void @@ -222,3 +222,17 @@ define void @test17v(i8* %P, i8* %Q) nounwind ssp { ; CHECK-NEXT: call void @llvm.memcpy ; CHECK-NEXT: ret } + +; PR8728 +; Do not delete instruction where possible situation is: +; A = B +; A = A +define void @test18(i8* %P, i8* %Q, i8* %R) nounwind ssp { + tail call void @llvm.memcpy.i64(i8* %P, i8* %Q, i64 12, i32 1) + tail call void @llvm.memcpy.i64(i8* %P, i8* %R, i64 12, i32 1) + ret void +; CHECK: @test18 +; CHECK-NEXT: call void @llvm.memcpy +; CHECK-NEXT: call void @llvm.memcpy +; CHECK-NEXT: ret +}