<div dir="ltr">This was reverted in <font color="#000000"><span style="white-space:pre-wrap">r193955, it caused PR17781.</span></font><div><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div><div><font color="#000000"><span style="white-space:pre-wrap">This introduces a regression if the caller byval </span></font><span style="color:rgb(0,0,0);white-space:pre-wrap">argument</span><font color="#000000"><span style="white-space:pre-wrap"> was accessible through other means, such as being global.</span></font></div>
<div><font color="#000000"><span style="white-space:pre-wrap"><br></span></font></div><div><span style="white-space:pre-wrap;color:rgb(0,0,0)">If we stored against the global and then loaded from the argument, we *should* get the original value, not the modified one.</span><br>
</div><div><font color="#000000"><span style="white-space:pre-wrap"><br></span></font></div><div><font color="#000000"><span style="white-space:pre-wrap">Simply checking that the argument is const is insufficient to ensure that eliding the copy is safe.</span></font></div>
<div><font color="#000000"><span style="white-space:pre-wrap"><br></span></font></div><div><font color="#000000"><span style="white-space:pre-wrap">A reduced test case for this specific example is part of </span></font><span style="color:rgb(0,0,0);white-space:pre-wrap">r193955 as well.</span></div>
<div><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Oct 24, 2013 at 9:38 AM, Tom Stellard <span dir="ltr"><<a href="mailto:thomas.stellard@amd.com" target="_blank">thomas.stellard@amd.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: tstellar<br>
Date: Thu Oct 24 11:38:33 2013<br>
New Revision: 193356<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=193356&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=193356&view=rev</a><br>
Log:<br>
Inliner: Handle readonly attribute per argument when adding memcpy<br>
<br>
Patch by: Vincent Lejeune<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp<br>
llvm/trunk/test/Transforms/Inline/byval.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp?rev=193356&r1=193355&r2=193356&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp?rev=193356&r1=193355&r2=193356&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp Thu Oct 24 11:38:33 2013<br>
@@ -337,33 +337,35 @@ static void UpdateCallGraphAfterInlining<br>
<br>
/// HandleByValArgument - When inlining a call site that has a byval argument,<br>
/// we have to make the implicit memcpy explicit by adding it.<br>
-static Value *HandleByValArgument(Value *Arg, Instruction *TheCall,<br>
+static Value *HandleByValArgument(Value *PassedValue,<br>
+ const Argument *ArgumentSignature,<br>
+ Instruction *TheCall,<br>
const Function *CalledFunc,<br>
InlineFunctionInfo &IFI,<br>
unsigned ByValAlignment) {<br>
- Type *AggTy = cast<PointerType>(Arg->getType())->getElementType();<br>
+ Type *AggTy = cast<PointerType>(PassedValue->getType())->getElementType();<br>
<br>
// If the called function is readonly, then it could not mutate the caller's<br>
// copy of the byval'd memory. In this case, it is safe to elide the copy and<br>
// temporary.<br>
- if (CalledFunc->onlyReadsMemory()) {<br>
+ if (CalledFunc->onlyReadsMemory() || ArgumentSignature->onlyReadsMemory()) {<br>
// If the byval argument has a specified alignment that is greater than the<br>
// passed in pointer, then we either have to round up the input pointer or<br>
// give up on this transformation.<br>
if (ByValAlignment <= 1) // 0 = unspecified, 1 = no particular alignment.<br>
- return Arg;<br>
+ return PassedValue;<br>
<br>
// If the pointer is already known to be sufficiently aligned, or if we can<br>
// round it up to a larger alignment, then we don't need a temporary.<br>
- if (getOrEnforceKnownAlignment(Arg, ByValAlignment,<br>
+ if (getOrEnforceKnownAlignment(PassedValue, ByValAlignment,<br>
<a href="http://IFI.TD" target="_blank">IFI.TD</a>) >= ByValAlignment)<br>
- return Arg;<br>
+ return PassedValue;<br>
<br>
// Otherwise, we have to make a memcpy to get a safe alignment. This is bad<br>
// for code quality, but rarely happens and is required for correctness.<br>
}<br>
<br>
- LLVMContext &Context = Arg->getContext();<br>
+ LLVMContext &Context = PassedValue->getContext();<br>
<br>
Type *VoidPtrTy = Type::getInt8PtrTy(Context);<br>
<br>
@@ -379,7 +381,7 @@ static Value *HandleByValArgument(Value<br>
<br>
Function *Caller = TheCall->getParent()->getParent();<br>
<br>
- Value *NewAlloca = new AllocaInst(AggTy, 0, Align, Arg->getName(),<br>
+ Value *NewAlloca = new AllocaInst(AggTy, 0, Align, PassedValue->getName(),<br>
&*Caller->begin()->begin());<br>
// Emit a memcpy.<br>
Type *Tys[3] = {VoidPtrTy, VoidPtrTy, Type::getInt64Ty(Context)};<br>
@@ -387,7 +389,7 @@ static Value *HandleByValArgument(Value<br>
Intrinsic::memcpy,<br>
Tys);<br>
Value *DestCast = new BitCastInst(NewAlloca, VoidPtrTy, "tmp", TheCall);<br>
- Value *SrcCast = new BitCastInst(Arg, VoidPtrTy, "tmp", TheCall);<br>
+ Value *SrcCast = new BitCastInst(PassedValue, VoidPtrTy, "tmp", TheCall);<br>
<br>
Value *Size;<br>
if (<a href="http://IFI.TD" target="_blank">IFI.TD</a> == 0)<br>
@@ -588,13 +590,14 @@ bool llvm::InlineFunction(CallSite CS, I<br>
for (Function::const_arg_iterator I = CalledFunc->arg_begin(),<br>
E = CalledFunc->arg_end(); I != E; ++I, ++AI, ++ArgNo) {<br>
Value *ActualArg = *AI;<br>
+ const Argument *Arg = I;<br>
<br>
// When byval arguments actually inlined, we need to make the copy implied<br>
// by them explicit. However, we don't do this if the callee is readonly<br>
// or readnone, because the copy would be unneeded: the callee doesn't<br>
// modify the struct.<br>
if (CS.isByValArgument(ArgNo)) {<br>
- ActualArg = HandleByValArgument(ActualArg, TheCall, CalledFunc, IFI,<br>
+ ActualArg = HandleByValArgument(ActualArg, Arg, TheCall, CalledFunc, IFI,<br>
CalledFunc->getParamAlignment(ArgNo+1));<br>
<br>
// Calls that we inline may use the new alloca, so we need to clear<br>
<br>
Modified: llvm/trunk/test/Transforms/Inline/byval.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/byval.ll?rev=193356&r1=193355&r2=193356&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/byval.ll?rev=193356&r1=193355&r2=193356&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/Inline/byval.ll (original)<br>
+++ llvm/trunk/test/Transforms/Inline/byval.ll Thu Oct 24 11:38:33 2013<br>
@@ -25,7 +25,7 @@ entry:<br>
store i64 2, i64* %tmp4, align 4<br>
call void @f( %struct.ss* byval %S ) nounwind<br>
ret i32 0<br>
-; CHECK: @test1()<br>
+; CHECK-LABEL: @test1()<br>
; CHECK: %S1 = alloca %struct.ss<br>
; CHECK: %S = alloca %struct.ss<br>
; CHECK: call void @llvm.memcpy<br>
@@ -52,7 +52,7 @@ entry:<br>
store i64 2, i64* %tmp4, align 4<br>
%X = call i32 @f2( %struct.ss* byval %S ) nounwind<br>
ret i32 %X<br>
-; CHECK: @test2()<br>
+; CHECK-LABEL: @test2()<br>
; CHECK: %S = alloca %struct.ss<br>
; CHECK-NOT: call void @llvm.memcpy<br>
; CHECK: ret i32<br>
@@ -74,7 +74,7 @@ entry:<br>
%S = alloca %struct.ss, align 1 ;; May not be aligned.<br>
call void @f3( %struct.ss* byval align 64 %S) nounwind<br>
ret void<br>
-; CHECK: @test3()<br>
+; CHECK-LABEL: @test3()<br>
; CHECK: %S1 = alloca %struct.ss, align 64<br>
; CHECK: %S = alloca %struct.ss<br>
; CHECK: call void @llvm.memcpy<br>
@@ -97,10 +97,35 @@ entry:<br>
%S = alloca %struct.ss, align 2 ; <%struct.ss*> [#uses=4]<br>
%X = call i32 @f4( %struct.ss* byval align 64 %S ) nounwind<br>
ret i32 %X<br>
-; CHECK: @test4()<br>
+; CHECK-LABEL: @test4()<br>
; CHECK: %S = alloca %struct.ss, align 64<br>
; CHECK-NOT: call void @llvm.memcpy<br>
; CHECK: call void @g3<br>
; CHECK: ret i32 4<br>
}<br>
<br>
+; Inlining a byval struct should NOT cause an explicit copy<br>
+; into an alloca if the parameter is readonly<br>
+<br>
+define internal i32 @f5(%struct.ss* byval readonly %b) nounwind {<br>
+entry:<br>
+ %tmp = getelementptr %struct.ss* %b, i32 0, i32 0 ; <i32*> [#uses=2]<br>
+ %tmp1 = load i32* %tmp, align 4 ; <i32> [#uses=1]<br>
+ %tmp2 = add i32 %tmp1, 1 ; <i32> [#uses=1]<br>
+ ret i32 %tmp2<br>
+}<br>
+<br>
+define i32 @test5() nounwind {<br>
+entry:<br>
+ %S = alloca %struct.ss ; <%struct.ss*> [#uses=4]<br>
+ %tmp1 = getelementptr %struct.ss* %S, i32 0, i32 0 ; <i32*> [#uses=1]<br>
+ store i32 1, i32* %tmp1, align 8<br>
+ %tmp4 = getelementptr %struct.ss* %S, i32 0, i32 1 ; <i64*> [#uses=1]<br>
+ store i64 2, i64* %tmp4, align 4<br>
+ %X = call i32 @f5( %struct.ss* byval %S ) nounwind<br>
+ ret i32 %X<br>
+; CHECK-LABEL: @test5()<br>
+; CHECK: %S = alloca %struct.ss<br>
+; CHECK-NOT: call void @llvm.memcpy<br>
+; CHECK: ret i32<br>
+}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>