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

Arnold aschwaighofer at apple.com
Sun Jul 19 12:16:18 PDT 2015 


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