PATCH: Inliner: Don't generate memcpy calls for byval readonly parameters
Tom Stellard
tom at stellard.net
Wed Oct 23 15:54:18 PDT 2013
On Wed, Oct 23, 2013 at 05:52:26AM -0700, Vincent Lejeune wrote:
> Thank for adding some test to my patch !
> However is it possible to change the name of the HandleByValArgument ?
> I used ArgAsArgument but actually Arg and ArgAsArgument represent 2 different things,
> Arg is the value passed to the function call, while ArgAsArgument is indeed the argument
> in the called signature. I think renaming Arg to "PassedValue" and ArgAsValue to
> "ArgumentSignature" would make the description of both parameters more self
> explanatory.
>
Here is an updated version of the patch, does this look OK to everyone?
-Tom
> Vincent
>
>
> -static Value *HandleByValArgument(Value *Arg, Instruction *TheCall,
> +static Value *HandleByValArgument(Value *Arg,
> + const Argument *ArgAsArgument,
> + Instruction *TheCall,
>
>
>
>
>
>
>
> >________________________________
> > De : Nick Lewycky <nlewycky at google.com>
> >À : Chandler Carruth <chandlerc at google.com>
> >Cc : Tom Stellard <tom at stellard.net>; Commit Messages and Patches for LLVM <llvm-commits at cs.uiuc.edu>; vljn at ovi.com
> >Envoyé le : Mercredi 23 octobre 2013 0h20
> >Objet : Re: PATCH: Inliner: Don't generate memcpy calls for byval readonly parameters
> >
> >
> >
> >On 22 October 2013 15:11, Chandler Carruth <chandlerc at google.com> wrote:
> >
> >This looks fine, but Nick should confirm that it is the correct implementation of byval & readonly.
> >
> >
> >I don't deeply understand 'byval', but this patch looks like it can't possibly be wrong.
> >
> >
> >On Tue, Oct 22, 2013 at 3:07 PM, Tom Stellard <tom at stellard.net> wrote:
> >>
> >>Hi,
> >>>
> >>>The function inliner avoids generating memcpy calls when the parameter
> >>>is marked byval and the function is marked readonly. The attached
> >>>patch extends this behavior to the case where the function parameter is
> >>>readonly, but the function isn't.
> >>>
> >>>Please review.
> >>>
> >>>Thanks,
> >>>Tom
> >>>
> >>
> >
> >
> >
> >
-------------- next part --------------
>From 0a7612bb0f8d70fe3f62140a005fdbe2b16e42d6 Mon Sep 17 00:00:00 2001
From: Vincent Lejeune <vljn at ovi.com>
Date: Tue, 22 Oct 2013 00:18:58 +0200
Subject: [PATCH] Inliner: Handle readonly attribute per argument when adding
memcpy
Patch by: Vincent Lejeune
---
lib/Transforms/Utils/InlineFunction.cpp | 23 +++++++++++++----------
test/Transforms/Inline/byval.ll | 33 +++++++++++++++++++++++++++++----
2 files changed, 42 insertions(+), 14 deletions(-)
diff --git a/lib/Transforms/Utils/InlineFunction.cpp b/lib/Transforms/Utils/InlineFunction.cpp
index dabb67b..47198c1 100644
--- a/lib/Transforms/Utils/InlineFunction.cpp
+++ b/lib/Transforms/Utils/InlineFunction.cpp
@@ -337,33 +337,35 @@ static void UpdateCallGraphAfterInlining(CallSite CS,
/// HandleByValArgument - When inlining a call site that has a byval argument,
/// we have to make the implicit memcpy explicit by adding it.
-static Value *HandleByValArgument(Value *Arg, Instruction *TheCall,
+static Value *HandleByValArgument(Value *PassedValue,
+ const Argument *ArgumentSignature,
+ Instruction *TheCall,
const Function *CalledFunc,
InlineFunctionInfo &IFI,
unsigned ByValAlignment) {
- Type *AggTy = cast<PointerType>(Arg->getType())->getElementType();
+ Type *AggTy = cast<PointerType>(PassedValue->getType())->getElementType();
// If the called function is readonly, then it could not mutate the caller's
// copy of the byval'd memory. In this case, it is safe to elide the copy and
// temporary.
- if (CalledFunc->onlyReadsMemory()) {
+ if (CalledFunc->onlyReadsMemory() || ArgumentSignature->onlyReadsMemory()) {
// If the byval argument has a specified alignment that is greater than the
// passed in pointer, then we either have to round up the input pointer or
// give up on this transformation.
if (ByValAlignment <= 1) // 0 = unspecified, 1 = no particular alignment.
- return Arg;
+ return PassedValue;
// If the pointer is already known to be sufficiently aligned, or if we can
// round it up to a larger alignment, then we don't need a temporary.
- if (getOrEnforceKnownAlignment(Arg, ByValAlignment,
+ if (getOrEnforceKnownAlignment(PassedValue, ByValAlignment,
IFI.TD) >= ByValAlignment)
- return Arg;
+ return PassedValue;
// Otherwise, we have to make a memcpy to get a safe alignment. This is bad
// for code quality, but rarely happens and is required for correctness.
}
- LLVMContext &Context = Arg->getContext();
+ LLVMContext &Context = PassedValue->getContext();
Type *VoidPtrTy = Type::getInt8PtrTy(Context);
@@ -379,7 +381,7 @@ static Value *HandleByValArgument(Value *Arg, Instruction *TheCall,
Function *Caller = TheCall->getParent()->getParent();
- Value *NewAlloca = new AllocaInst(AggTy, 0, Align, Arg->getName(),
+ Value *NewAlloca = new AllocaInst(AggTy, 0, Align, PassedValue->getName(),
&*Caller->begin()->begin());
// Emit a memcpy.
Type *Tys[3] = {VoidPtrTy, VoidPtrTy, Type::getInt64Ty(Context)};
@@ -387,7 +389,7 @@ static Value *HandleByValArgument(Value *Arg, Instruction *TheCall,
Intrinsic::memcpy,
Tys);
Value *DestCast = new BitCastInst(NewAlloca, VoidPtrTy, "tmp", TheCall);
- Value *SrcCast = new BitCastInst(Arg, VoidPtrTy, "tmp", TheCall);
+ Value *SrcCast = new BitCastInst(PassedValue, VoidPtrTy, "tmp", TheCall);
Value *Size;
if (IFI.TD == 0)
@@ -588,13 +590,14 @@ bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
for (Function::const_arg_iterator I = CalledFunc->arg_begin(),
E = CalledFunc->arg_end(); I != E; ++I, ++AI, ++ArgNo) {
Value *ActualArg = *AI;
+ const Argument *Arg = I;
// When byval arguments actually inlined, we need to make the copy implied
// by them explicit. However, we don't do this if the callee is readonly
// or readnone, because the copy would be unneeded: the callee doesn't
// modify the struct.
if (CS.isByValArgument(ArgNo)) {
- ActualArg = HandleByValArgument(ActualArg, TheCall, CalledFunc, IFI,
+ ActualArg = HandleByValArgument(ActualArg, Arg, TheCall, CalledFunc, IFI,
CalledFunc->getParamAlignment(ArgNo+1));
// Calls that we inline may use the new alloca, so we need to clear
diff --git a/test/Transforms/Inline/byval.ll b/test/Transforms/Inline/byval.ll
index e601faf..aceae11 100644
--- a/test/Transforms/Inline/byval.ll
+++ b/test/Transforms/Inline/byval.ll
@@ -25,7 +25,7 @@ entry:
store i64 2, i64* %tmp4, align 4
call void @f( %struct.ss* byval %S ) nounwind
ret i32 0
-; CHECK: @test1()
+; CHECK-LABEL: @test1()
; CHECK: %S1 = alloca %struct.ss
; CHECK: %S = alloca %struct.ss
; CHECK: call void @llvm.memcpy
@@ -52,7 +52,7 @@ entry:
store i64 2, i64* %tmp4, align 4
%X = call i32 @f2( %struct.ss* byval %S ) nounwind
ret i32 %X
-; CHECK: @test2()
+; CHECK-LABEL: @test2()
; CHECK: %S = alloca %struct.ss
; CHECK-NOT: call void @llvm.memcpy
; CHECK: ret i32
@@ -74,7 +74,7 @@ entry:
%S = alloca %struct.ss, align 1 ;; May not be aligned.
call void @f3( %struct.ss* byval align 64 %S) nounwind
ret void
-; CHECK: @test3()
+; CHECK-LABEL: @test3()
; CHECK: %S1 = alloca %struct.ss, align 64
; CHECK: %S = alloca %struct.ss
; CHECK: call void @llvm.memcpy
@@ -97,10 +97,35 @@ entry:
%S = alloca %struct.ss, align 2 ; <%struct.ss*> [#uses=4]
%X = call i32 @f4( %struct.ss* byval align 64 %S ) nounwind
ret i32 %X
-; CHECK: @test4()
+; CHECK-LABEL: @test4()
; CHECK: %S = alloca %struct.ss, align 64
; CHECK-NOT: call void @llvm.memcpy
; CHECK: call void @g3
; CHECK: ret i32 4
}
+; Inlining a byval struct should NOT cause an explicit copy
+; into an alloca if the parameter is readonly
+
+define internal i32 @f5(%struct.ss* byval readonly %b) nounwind {
+entry:
+ %tmp = getelementptr %struct.ss* %b, i32 0, i32 0 ; <i32*> [#uses=2]
+ %tmp1 = load i32* %tmp, align 4 ; <i32> [#uses=1]
+ %tmp2 = add i32 %tmp1, 1 ; <i32> [#uses=1]
+ ret i32 %tmp2
+}
+
+define i32 @test5() nounwind {
+entry:
+ %S = alloca %struct.ss ; <%struct.ss*> [#uses=4]
+ %tmp1 = getelementptr %struct.ss* %S, i32 0, i32 0 ; <i32*> [#uses=1]
+ store i32 1, i32* %tmp1, align 8
+ %tmp4 = getelementptr %struct.ss* %S, i32 0, i32 1 ; <i64*> [#uses=1]
+ store i64 2, i64* %tmp4, align 4
+ %X = call i32 @f5( %struct.ss* byval %S ) nounwind
+ ret i32 %X
+; CHECK-LABEL: @test5()
+; CHECK: %S = alloca %struct.ss
+; CHECK-NOT: call void @llvm.memcpy
+; CHECK: ret i32
+}
--
1.7.11.4
More information about the llvm-commits
mailing list