[llvm] r242558 - MergeFuncs: Transfer the function parameter attributes to the call site
Arnold
aschwaighofer at apple.com
Sun Jul 19 11:13:19 PDT 2015
Sent from my iPhone
> On Jul 19, 2015, at 10:55 AM, Pete Cooper <peter_cooper at apple.com> wrote:
>
>
>> On Jul 19, 2015, at 10:45 AM, Arnold <aschwaighofer at apple.com> wrote:
>>
>> 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.
> Absolutely.
>
> I think the problem is that because you don’t know if everyone querying the call attributes will use paramHasAttr or getAttributes().hasAttribute(), you have to defensively apply everything to the call inst, even if its also on the Function*.
>
Some clients might want the numeric values of the attribute (e.g MergeFunctions) - querying for the presence only to get the value seems an unnecessary indirection -that's why I suggested doing it at the getAttributes layer. As you correctly point out such union has to make semantic sense.
> But saying that, it works to have them on the call, so not sure if this is really that much of a problem.
>>
>> 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.
> It probably should be the union, probably. There would be cases where thats legal, and cases where it isn’t, depending on what the call attributes are vs the function attributes. Imagine zext on the call return vs sext on the function return for example. I would say thats illegal.
>
> One solution is to say that the call attributes are required to be the superset of the function. The verifier could check that if we want it to.
Yes - the union has to make semantic sense.
> Pete
>>
>> 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/f1db551c/attachment.html>
More information about the llvm-commits
mailing list