[llvm] r242558 - MergeFuncs: Transfer the function parameter attributes to the call site

Nick Lewycky nicholas at mxc.ca
Sun Jul 19 12:03:40 PDT 2015 

Pete Cooper wrote:
>
>> On Jul 19, 2015, at 10:45 AM, Arnold <aschwaighofer at apple.com
>> <mailto: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.

Yes, there is a surprise in LLVM IR: if you take a direct call and turn 
it into an indirect call on the same callee, you may change how that 
call is made in ABI-affecting ways. This is just a fact of life for 
whoever wants to do such a transform. However, this isn't the case 
regarding this commit to mergefunc.

> 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*.

Code which calls getAttributes() when it meant paramHasAttr() is buggy. 
There does need to be a lower-level API that accesses the CallInst but 
not the Function, or else things like .ll printing aren't possible. 
That's what getAttributes() is for.

> But saying that, it works to have them on the call, so not sure if this
> is really that much of a problem.

I strongly suspect that if this patch fixed anything, it is working 
around another bug. That's what I'm worried about. Please revert this 
patch and fix that other bug instead.

Nick

>> 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.
>
> Pete
>>
>> Sent from my iPhone
>>
>> On Jul 19, 2015, at 10:00 AM, Pete Cooper <peter_cooper at apple.com
>> <mailto:peter_cooper at apple.com>> wrote:
>>
>>>
>>>> On Jul 19, 2015, at 7:38 AM, Arnold <aschwaighofer at apple.com
>>>> <mailto: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:
>>>>
>>>> 1.
>>>>     ‘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))
>>> returntrue;
>>> if(constFunction*F = getCalledFunction())
>>> returnF->getAttributes().hasAttribute(i, A);
>>> returnfalse;
>>> }
>>>
>>> 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
>>>> <mailto: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
>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D242558-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=DPCjTli64sYglbnTnKC7MvdjwE6U5MSXljsFzmbuDcI&s=AU3JkYwjgEN23h8zeuGg2xvw_Pd_urPbuK__Q1zpI1o&e=>
>>>>>> 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
>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Transforms_IPO_MergeFunctions.cpp-3Frev-3D242558-26r1-3D242557-26r2-3D242558-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=DPCjTli64sYglbnTnKC7MvdjwE6U5MSXljsFzmbuDcI&s=0hHQ64Km6Zc5uC4vIBmQq0tUWM0hFX9WbzYZO6mS9pQ&e=>
>>>>>> ==============================================================================
>>>>>> --- 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
>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_Transforms_MergeFunc_apply-5Ffunction-5Fattributes.ll-3Frev-3D242558-26view-3Dauto&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=DPCjTli64sYglbnTnKC7MvdjwE6U5MSXljsFzmbuDcI&s=oDyFKdPX-IJxjHbcYQt2q4s1WX6KAWrsmnOAsKRZwss&e=>
>>>>>> ==============================================================================
>>>>>> ---
>>>>>> 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
>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_Transforms_MergeFunc_inttoptr-2Daddress-2Dspace.ll-3Frev-3D242558-26r1-3D242557-26r2-3D242558-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=DPCjTli64sYglbnTnKC7MvdjwE6U5MSXljsFzmbuDcI&s=grRo64gg6cEddLXKnGXBJMC_64FfYrkO5rYJUbAZFMw&e=>
>>>>>> ==============================================================================
>>>>>> --- 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
>>>>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_Transforms_MergeFunc_inttoptr.ll-3Frev-3D242558-26r1-3D242557-26r2-3D242558-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=DPCjTli64sYglbnTnKC7MvdjwE6U5MSXljsFzmbuDcI&s=lsJYKtLqvqAdKntbddF14cNqHGmPl4WbOPQB3D4SkMA&e=>
>>>>>> ==============================================================================
>>>>>> --- 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 <mailto: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 <mailto:llvm-commits at cs.uiuc.edu>
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>

More information about the llvm-commits mailing list