[llvm-commits] [llvm] r120974 - in /llvm/trunk: include/llvm/Analysis/AliasAnalysis.h lib/Transforms/Scalar/DeadStoreElimination.cpp test/Transforms/DeadStoreElimination/simple.ll
Chris Lattner
sabre at nondot.org
Sun Dec 5 17:48:06 PST 2010
Author: lattner
Date: Sun Dec 5 19:48:06 2010
New Revision: 120974
URL: http://llvm.org/viewvc/llvm-project?rev=120974&view=rev
Log:
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!
Modified:
llvm/trunk/include/llvm/Analysis/AliasAnalysis.h
llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp
llvm/trunk/test/Transforms/DeadStoreElimination/simple.ll
Modified: llvm/trunk/include/llvm/Analysis/AliasAnalysis.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/AliasAnalysis.h?rev=120974&r1=120973&r2=120974&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Analysis/AliasAnalysis.h (original)
+++ llvm/trunk/include/llvm/Analysis/AliasAnalysis.h Sun Dec 5 19:48:06 2010
@@ -188,6 +188,11 @@
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
Modified: llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp?rev=120974&r1=120973&r2=120974&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp Sun Dec 5 19:48:06 2010
@@ -198,6 +198,20 @@
}
}
+/// 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<MemTransferInst>(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 @@
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 @@
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 @@
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
Modified: llvm/trunk/test/Transforms/DeadStoreElimination/simple.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadStoreElimination/simple.ll?rev=120974&r1=120973&r2=120974&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/DeadStoreElimination/simple.ll (original)
+++ llvm/trunk/test/Transforms/DeadStoreElimination/simple.ll Sun Dec 5 19:48:06 2010
@@ -203,7 +203,7 @@
}
;; 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 @@
; 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
+}
More information about the llvm-commits
mailing list