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

[Nick Lewycky]

Arnold Schwaighofer wrote:
>
>> On Jul 19, 2015, at 12:54 PM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>>
>> 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.
>
>
> I was interpreting the following sentence as requiring a call to have the parameter attributes:
>
> http://llvm.org/docs/LangRef.html#call-instruction
>
> 7. ‘function args‘: argument list whose types match the function signature argument types and parameter attributes

Yeah, that says exactly what you think it does.

But it can't be right. :) We add things like nocapture/readnone/readonly 
etc., all the time, and there's no way to even update all callers if we 
wanted to. What if they're in other Modules?

Taking a look at the history, it was added in r97557: 
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20100301/097129.html 
. Previously it was part of the type, which actually makes quite some 
sense to me if we need to know it in order to place the call.

It sounds like there's some tension between the way I've been thinking 
about attributes, as optimization hints, and people who think about them 
as essential ABI issues, since they are used for both. Fixing may go any 
number of ways:
  - just remove the words 'and parameter attributes' from point 7 of 
call inst
  - split the definition of parameter attributes into those that are ABI 
attributes and those that are not. Require the ABI ones to be present on 
all calls.
  - require that the CallInst hold a superset of the parameters on the 
called Function. Update CallInsts when the module linker merges modules 
where one Function has more attributes on it than the other.
I think all of these would be internally consistent.

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