<div dir="ltr">Hello,<div><br></div><div>I'm seeing miscompiles at -O2 that bisect to this revision. I'm working on reducing a test case now... it looks to my (admittedly unfamiliar) eyes like this revision may not correctly handle structs correctly if they are alloca'ed.</div><div><br></div><div>I'll update when I have more (or a test case).</div><div><br></div><div>--dlj</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Dec 7, 2018 at 1:19 PM Nikita Popov 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: nikic<br>
Date: Fri Dec 7 13:16:58 2018<br>
New Revision: 348645<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=348645&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=348645&view=rev</a><br>
Log:<br>
[MemCpyOpt] memset->memcpy forwarding with undef tail<br>
<br>
Currently memcpyopt optimizes cases like<br>
<br>
memset(a, byte, N);<br>
memcpy(b, a, M);<br>
<br>
to<br>
<br>
memset(a, byte, N);<br>
memset(b, byte, M);<br>
<br>
if M <= N. Often this allows further simplifications down the line,<br>
which drop the first memset entirely.<br>
<br>
This patch extends this optimization for the case where M > N, but we<br>
know that the bytes a[N..M] are undef due to alloca/lifetime.start.<br>
<br>
This situation arises relatively often for Rust code, because Rust does<br>
not initialize trailing structure padding and loves to insert redundant<br>
memcpys. This also fixes <a href="https://bugs.llvm.org/show_bug.cgi?id=39844" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=39844</a>.<br>
<br>
For the implementation, I'm reusing a bit of code for a similar existing<br>
optimization (direct memcpy of undef). I've also added memset support to<br>
MemDepAnalysis GetLocation -- Instead, getPointerDependencyFrom could be<br>
used, but it seems to make more sense to add this to GetLocation and thus<br>
make the computation cachable.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D55120" rel="noreferrer" target="_blank">https://reviews.llvm.org/D55120</a><br>
<br>
Modified:<br>
llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp<br>
llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp<br>
llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-oversized.ll<br>
<br>
Modified: llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp?rev=348645&r1=348644&r2=348645&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp?rev=348645&r1=348644&r2=348645&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp Fri Dec 7 13:16:58 2018<br>
@@ -154,6 +154,12 @@ static ModRefInfo GetLocation(const Inst<br>
return ModRefInfo::Mod;<br>
}<br>
<br>
+ if (const MemSetInst *MI = dyn_cast<MemSetInst>(Inst)) {<br>
+ Loc = MemoryLocation::getForDest(MI);<br>
+ // Conversatively assume ModRef for volatile memset.<br>
+ return MI->isVolatile() ? ModRefInfo::ModRef : ModRefInfo::Mod;<br>
+ }<br>
+<br>
if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {<br>
switch (II->getIntrinsicID()) {<br>
case Intrinsic::lifetime_start:<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp?rev=348645&r1=348644&r2=348645&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp?rev=348645&r1=348644&r2=348645&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp Fri Dec 7 13:16:58 2018<br>
@@ -1144,6 +1144,21 @@ bool MemCpyOptPass::processMemSetMemCpyD<br>
return true;<br>
}<br>
<br>
+/// Determine whether the instruction has undefined content for the given Size,<br>
+/// either because it was freshly alloca'd or started its lifetime.<br>
+static bool hasUndefContents(Instruction *I, ConstantInt *Size) {<br>
+ if (isa<AllocaInst>(I))<br>
+ return true;<br>
+<br>
+ if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I))<br>
+ if (II->getIntrinsicID() == Intrinsic::lifetime_start)<br>
+ if (ConstantInt *LTSize = dyn_cast<ConstantInt>(II->getArgOperand(0)))<br>
+ if (LTSize->getZExtValue() >= Size->getZExtValue())<br>
+ return true;<br>
+<br>
+ return false;<br>
+}<br>
+<br>
/// Transform memcpy to memset when its source was just memset.<br>
/// In other words, turn:<br>
/// \code<br>
@@ -1167,12 +1182,23 @@ bool MemCpyOptPass::performMemCpyToMemSe<br>
if (!AA.isMustAlias(MemSet->getRawDest(), MemCpy->getRawSource()))<br>
return false;<br>
<br>
- ConstantInt *CopySize = cast<ConstantInt>(MemCpy->getLength());<br>
+ // A known memset size is required.<br>
ConstantInt *MemSetSize = dyn_cast<ConstantInt>(MemSet->getLength());<br>
+ if (!MemSetSize)<br>
+ return false;<br>
+<br>
// Make sure the memcpy doesn't read any more than what the memset wrote.<br>
// Don't worry about sizes larger than i64.<br>
- if (!MemSetSize || CopySize->getZExtValue() > MemSetSize->getZExtValue())<br>
- return false;<br>
+ ConstantInt *CopySize = cast<ConstantInt>(MemCpy->getLength());<br>
+ if (CopySize->getZExtValue() > MemSetSize->getZExtValue()) {<br>
+ // If the memcpy is larger than the memset, but the memory was undef prior<br>
+ // to the memset, we can just ignore the tail.<br>
+ MemDepResult DepInfo = MD->getDependency(MemSet);<br>
+ if (DepInfo.isDef() && hasUndefContents(DepInfo.getInst(), CopySize))<br>
+ CopySize = MemSetSize;<br>
+ else<br>
+ return false;<br>
+ }<br>
<br>
IRBuilder<> Builder(MemCpy);<br>
Builder.CreateMemSet(MemCpy->getRawDest(), MemSet->getOperand(1),<br>
@@ -1252,19 +1278,7 @@ bool MemCpyOptPass::processMemCpy(MemCpy<br>
if (MemCpyInst *MDep = dyn_cast<MemCpyInst>(SrcDepInfo.getInst()))<br>
return processMemCpyMemCpyDependence(M, MDep);<br>
} else if (SrcDepInfo.isDef()) {<br>
- Instruction *I = SrcDepInfo.getInst();<br>
- bool hasUndefContents = false;<br>
-<br>
- if (isa<AllocaInst>(I)) {<br>
- hasUndefContents = true;<br>
- } else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {<br>
- if (II->getIntrinsicID() == Intrinsic::lifetime_start)<br>
- if (ConstantInt *LTSize = dyn_cast<ConstantInt>(II->getArgOperand(0)))<br>
- if (LTSize->getZExtValue() >= CopySize->getZExtValue())<br>
- hasUndefContents = true;<br>
- }<br>
-<br>
- if (hasUndefContents) {<br>
+ if (hasUndefContents(SrcDepInfo.getInst(), CopySize)) {<br>
MD->removeInstruction(M);<br>
M->eraseFromParent();<br>
++NumMemCpyInstr;<br>
<br>
Modified: llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-oversized.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-oversized.ll?rev=348645&r1=348644&r2=348645&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-oversized.ll?rev=348645&r1=348644&r2=348645&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-oversized.ll (original)<br>
+++ llvm/trunk/test/Transforms/MemCpyOpt/memset-memcpy-oversized.ll Fri Dec 7 13:16:58 2018<br>
@@ -12,7 +12,7 @@ define void @test_alloca(i8* %result) {<br>
; CHECK-NEXT: [[A:%.*]] = alloca [[T:%.*]], align 8<br>
; CHECK-NEXT: [[B:%.*]] = bitcast %T* [[A]] to i8*<br>
; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* align 8 [[B]], i8 0, i64 12, i1 false)<br>
-; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* [[RESULT:%.*]], i8* align 8 [[B]], i64 16, i1 false)<br>
+; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[RESULT:%.*]], i8 0, i64 12, i1 false)<br>
; CHECK-NEXT: ret void<br>
;<br>
%a = alloca %T, align 8<br>
@@ -28,7 +28,7 @@ define void @test_alloca_with_lifetimes(<br>
; CHECK-NEXT: [[B:%.*]] = bitcast %T* [[A]] to i8*<br>
; CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 16, i8* [[B]])<br>
; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* align 8 [[B]], i8 0, i64 12, i1 false)<br>
-; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* [[RESULT:%.*]], i8* align 8 [[B]], i64 16, i1 false)<br>
+; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[RESULT:%.*]], i8 0, i64 12, i1 false)<br>
; CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 16, i8* [[B]])<br>
; CHECK-NEXT: ret void<br>
;<br>
@@ -46,7 +46,7 @@ define void @test_malloc_with_lifetimes(<br>
; CHECK-NEXT: [[A:%.*]] = call i8* @malloc(i64 16)<br>
; CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 16, i8* [[A]])<br>
; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* align 8 [[A]], i8 0, i64 12, i1 false)<br>
-; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* [[RESULT:%.*]], i8* align 8 [[A]], i64 16, i1 false)<br>
+; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[RESULT:%.*]], i8 0, i64 12, i1 false)<br>
; CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 16, i8* [[A]])<br>
; CHECK-NEXT: call void @free(i8* [[A]])<br>
; CHECK-NEXT: ret void<br>
@@ -98,7 +98,7 @@ define void @test_volatile_memset(i8* %r<br>
; CHECK-NEXT: [[A:%.*]] = alloca [[T:%.*]], align 8<br>
; CHECK-NEXT: [[B:%.*]] = bitcast %T* [[A]] to i8*<br>
; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* align 8 [[B]], i8 0, i64 12, i1 true)<br>
-; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* [[RESULT:%.*]], i8* align 8 [[B]], i64 16, i1 false)<br>
+; CHECK-NEXT: call void @llvm.memset.p0i8.i64(i8* [[RESULT:%.*]], i8 0, i64 12, i1 false)<br>
; CHECK-NEXT: ret void<br>
;<br>
%a = alloca %T, align 8<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>