[llvm] r242558 - MergeFuncs: Transfer the function parameter attributes to the call site
Arnold
aschwaighofer at apple.com
Sun Jul 19 10:45:57 PDT 2015
Given that we can call function pointers it seems to make sense to me to require to have (especially ABI changing) parameter attributes on the call site.
If that is not the case - that this is a requirement - and my interpretation of the langref is wrong - then the callinst's getattributes function should build the union of the callinst's attributes and the called function's attributes if available.
Sent from my iPhone
> On Jul 19, 2015, at 10:00 AM, Pete Cooper <peter_cooper at apple.com> wrote:
>
>
>> On Jul 19, 2015, at 7:38 AM, Arnold <aschwaighofer at apple.com> wrote:
>>
>> I am not sure what you are asking?
>>
>> Without transferring the attributes the call site will have no parameter attributes (of the callee functions).
> This doesn’t surprise me.
>>
>> My reading of the LangRef is that this should not be:
>> ‘function args‘: argument list whose types match the function signature argument types and parameter attributes.
>> Specifically, if the sret attribute (on some target it changes the abi) attribute is not applied to call sites) I have cases that fail after running this pass.
> The trouble with call site attributes is that not everyone calls methods like this one to query the call attributes:
>
> bool CallInst::paramHasAttr(unsigned i, Attribute::AttrKind A) const {
> if (AttributeList.hasAttribute(i, A))
> return true;
> if (const Function *F = getCalledFunction())
> return F->getAttributes().hasAttribute(i, A);
> return false;
> }
>
> Notice that it queries both the call site and the function attributes. So if someone adds and attribute to the call instruction you’ll see it here.
>
> However, if someone added an attribute to the Function* itself, then the call attributes are not updated. And so queries such as this one are not actually looking at the function attributes at all:
>
>
>
> bool DAE::RemoveDeadStuffFromFunction(Function *F) {
> …
> CallSite CS(F->user_back());
> Instruction *Call = CS.getInstruction();
> const AttributeSet &CallPAL = CS.getAttributes();
> if (CallPAL.hasAttributes(i + 1)) {
> AttrBuilder B(CallPAL, i + 1);
> // If the return type has changed, then get rid of 'returned' on the
> // call site. The alternative is to make all 'returned' attributes on
> // call sites keep the return value alive just like 'returned'
> // attributes on function declaration but it's less clearly a win
> // and this is not an expected case anyway
> if (NRetTy != RetTy && B.contains(Attribute::Returned))
> B.removeAttribute(Attribute::Returned);
> AttributesVec.
> push_back(AttributeSet::get(F->getContext(), Args.size(), B));
> }
>
>
> Personally I think the problem is that its even possible to do CallInst::getAttributes(). That leads to users accessing the attributes directly, possibly unaware that it doesn’t check the function attributes in that case.
>
> Cheers,
> Pete
>> Sent from my iPhone
>>
>>> On Jul 18, 2015, at 8:45 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
>>>
>>> Arnold Schwaighofer wrote:
>>>> Author: arnolds
>>>> Date: Fri Jul 17 13:59:08 2015
>>>> New Revision: 242558
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=242558&view=rev
>>>> Log:
>>>> MergeFuncs: Transfer the function parameter attributes to the call site
>>>>
>>>> rdar://21516488
>>>
>>> This doesn't make sense. I don't see any way that the call instruction gets any attributes that aren't already present on the callee function. Can you add a testcase where that isn't true?
>>>
>>> Nick
>>>
>>>>
>>>> Added:
>>>> llvm/trunk/test/Transforms/MergeFunc/apply_function_attributes.ll
>>>> Modified:
>>>> llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp
>>>> llvm/trunk/test/Transforms/MergeFunc/inttoptr-address-space.ll
>>>> llvm/trunk/test/Transforms/MergeFunc/inttoptr.ll
>>>>
>>>> Modified: llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp?rev=242558&r1=242557&r2=242558&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp (original)
>>>> +++ llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp Fri Jul 17 13:59:08 2015
>>>> @@ -1361,6 +1361,7 @@ void MergeFunctions::writeThunk(Function
>>>> CallInst *CI = Builder.CreateCall(F, Args);
>>>> CI->setTailCall();
>>>> CI->setCallingConv(F->getCallingConv());
>>>> + CI->setAttributes(F->getAttributes());
>>>> if (NewG->getReturnType()->isVoidTy()) {
>>>> Builder.CreateRetVoid();
>>>> } else {
>>>>
>>>> Added: llvm/trunk/test/Transforms/MergeFunc/apply_function_attributes.ll
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/MergeFunc/apply_function_attributes.ll?rev=242558&view=auto
>>>> ==============================================================================
>>>> --- llvm/trunk/test/Transforms/MergeFunc/apply_function_attributes.ll (added)
>>>> +++ llvm/trunk/test/Transforms/MergeFunc/apply_function_attributes.ll Fri Jul 17 13:59:08 2015
>>>> @@ -0,0 +1,22 @@
>>>> +; RUN: opt -S -mergefunc< %s | FileCheck %s
>>>> +%Si = type<{ i32 }>
>>>> +
>>>> +define void @sum(%Si* noalias sret %a, i32 %x, i32 %y) {
>>>> + %sum = add i32 %x, %y
>>>> + %sum2 = add i32 %sum, %y
>>>> + %sum3 = add i32 %sum2, %y
>>>> + ret void
>>>> +}
>>>> +
>>>> +define void @add(%Si* noalias sret %a, i32 %x, i32 %y) {
>>>> + %sum = add i32 %x, %y
>>>> + %sum2 = add i32 %sum, %y
>>>> + %sum3 = add i32 %sum2, %y
>>>> + ret void
>>>> +}
>>>> +
>>>> +; Make sure we transfer the parameter attributes to the call site.
>>>> +
>>>> +; CHECK-LABEL: define void @sum(%Si* noalias sret, i32, i32)
>>>> +; CHECK: tail call void @add(%Si* noalias sret %{{.*}}, i32 %{{.*}}, i32 %{{.*}})
>>>> +; CHECK: ret void
>>>>
>>>> Modified: llvm/trunk/test/Transforms/MergeFunc/inttoptr-address-space.ll
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/MergeFunc/inttoptr-address-space.ll?rev=242558&r1=242557&r2=242558&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/test/Transforms/MergeFunc/inttoptr-address-space.ll (original)
>>>> +++ llvm/trunk/test/Transforms/MergeFunc/inttoptr-address-space.ll Fri Jul 17 13:59:08 2015
>>>> @@ -21,7 +21,7 @@ define internal i8* @func35(%.qux.2585 a
>>>> bb:
>>>> ; CHECK-LABEL: @func35(
>>>> ; CHECK: %[[V2:.+]] = bitcast %.qux.2585 addrspace(1)* %{{.*}} to %.qux.2496 addrspace(1)*
>>>> -; CHECK: %[[V3:.+]] = tail call i32 @func10(%.qux.2496 addrspace(1)* %[[V2]])
>>>> +; CHECK: %[[V3:.+]] = tail call i32 @func10(%.qux.2496 addrspace(1)* nocapture %[[V2]])
>>>> ; CHECK: %{{.*}} = inttoptr i32 %[[V3]] to i8*
>>>> %tmp = getelementptr inbounds %.qux.2585, %.qux.2585 addrspace(1)* %this, i32 0, i32 2
>>>> %tmp1 = load i8*, i8* addrspace(1)* %tmp, align 4
>>>>
>>>> Modified: llvm/trunk/test/Transforms/MergeFunc/inttoptr.ll
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/MergeFunc/inttoptr.ll?rev=242558&r1=242557&r2=242558&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/test/Transforms/MergeFunc/inttoptr.ll (original)
>>>> +++ llvm/trunk/test/Transforms/MergeFunc/inttoptr.ll Fri Jul 17 13:59:08 2015
>>>> @@ -48,7 +48,7 @@ define internal i8* @func35(%.qux.2585*
>>>> bb:
>>>> ; CHECK-LABEL: @func35(
>>>> ; CHECK: %[[V2:.+]] = bitcast %.qux.2585* %{{.*}} to %.qux.2496*
>>>> -; CHECK: %[[V3:.+]] = tail call i32 @func10(%.qux.2496* %[[V2]])
>>>> +; CHECK: %[[V3:.+]] = tail call i32 @func10(%.qux.2496* nocapture %[[V2]])
>>>> ; CHECK: %{{.*}} = inttoptr i32 %[[V3]] to i8*
>>>> %tmp = getelementptr inbounds %.qux.2585, %.qux.2585* %this, i32 0, i32 2
>>>> %tmp1 = load i8*, i8** %tmp, align 4
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150719/d383619a/attachment.html>
More information about the llvm-commits
mailing list