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

Nick Lewycky nicholas at mxc.ca
Sun Jul 19 12:54:49 PDT 2015 

Arnold wrote:
>
>
> Sent from my iPhone
>
>> On Jul 19, 2015, at 12:03 PM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>>
>> 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.
>>
>
> So I misread the LangRef an transferring the parameter attributes is not required?

I think so. You may have a valid reading, but I don't know how you got 
there.

Looking at LangRef again, there is something new to me:
   * "All ABI-impacting function attributes, such as sret, byval, inreg, 
returned, and inalloca, must match."
but referring only to 'musttail' calls. So in that case, you would have 
to copy the param attrs! (I'm not sure why this rule is here, but your 
patch isn't the right place for me to argue changes to the langref.)

If you want to re-submit with a testcase for musttail, I think that 
would be a good bug fix.

Nick

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