[PATCH] Teach DeadArgElimination not to eliminate return values of functions with 'returned' arguments

[Jim Grosbach]

On Jun 27, 2013, at 4:28 PM, Nick Lewycky <nlewycky at google.com> wrote:

> On 26 June 2013 22:39, Evan Cheng <evan.cheng at apple.com> wrote:
>> 
>> 
>> Sent from my iPad
>> 
>> On Jun 26, 2013, at 7:53 PM, Nick Lewycky <nlewycky at google.com> wrote:
>> 
>>> On 26 June 2013 18:28, Evan Cheng <evan.cheng at apple.com> wrote:
>>>> 
>>>> On Jun 26, 2013, at 2:57 PM, Nick Lewycky <nlewycky at google.com> wrote:
>>>> 
>>>>>> 
>>>>>> >
>>>>>> > The 'returned' attribute is interprocedural register allocation done at the IR level, encoded with parameter attributes.
>>>>>> >
>>>>>> > This is a major architectural change to llvm. I don't think anybody realized the implications of the 'returned' patch when it was under review.
>>>>>> 
>>>>>> FYI, this was discussed inside the Apple team and Chris has approved this approach.
>>>>> 
>>>>> I appreciate that you did due diligence when designing it before sending it out. However, doing that design behind closed doors makes it unclear which objections have been raised or which concerns have been considered. There's a difference between "we saw the problems but decided this was the best tradeoff" and "we didn't see this problem". Indeed, your reply still hasn't told me which it was.
>>>> 
>>>> Nick, the 'returned' attribute was proposed and discussed in the mailing list. We had considered the implication of the patch carefully. I am not sure what you are getting at.
>>> 
>>> I read the discussion on the mailing list and my concerns weren't raised there. That's my fault. I saw an email with "ARMTargetLowering" and "SelectionDAGBuilder" and ignored it, assuming it was outside my domain.
>>> 
>>>>>> Also, I'm not sure why you consider this a "major architectural change".
>>>>> 
>>>>> Suppose I create a new parameter attribute 'AX' which means that this parameter will be present in register 'AX' upon function exit. What's the difference between this and attribute 'returned'? Clearly 'AX' is a reference to the x86 register set, while 'returned' means AX on some x86 calling conventions and other registers on other CCs or targets. But of course we could implement my attribute 'AX' differently on different targets. Is there any difference?
>>>> 
>>>> What? 'returned' is a target neutral attribute while 'AX' is x86 specific. I'm not following you at all.
>>> 
>>> My point is that 'returned' is just an alias for my 'AX' attribute -- if adding 'AX' constitutes IPA-RA then so does adding 'returned'. The actual chosen string doesn't matter (you wouldn't really suggest that by calling it register 1, register 2, etc., a register allocator becomes a target independent piece, suitable for the IR?).
>>> 
>>>>> Labelling llvm values with registers is register allocation. Doing register allocation on llvm IR, with llvm IR as output, is something I would consider a major architectural change.
>>>> 
>>>> We are discussing how DAE should deal with 'returned' arguments. The attribute just tells the optimizer the relationship between the function argument and the return value. This is not doing register allocation on LLVM ir. You are completely mis-characterizing this. It's not dramatically different from other attributes like sret, byval which are useful for describing some ABI properties in a target neutral manner.
>>> 
>>> It means "put this value in that register!" to the callee and "assume this value comes back in that register!" to the caller. The only thing stopping it from being an IPA-RA solution is the fact that it only applies to one register/argument. Remove that check in the verifier and give the backend a list of returning registers and -- blammo! -- you've got support for full blown interprocedural register allocation, just plug in your heuristics. In my mind, the fact that it only applies to one register does not save it from this criticism.
>> 
>> I disagree with your assessment. The attribute is not forcing the value into a specific register. It's merely informing codegen the value would be in a register that is enforced by the calling convention. It's a very fine distinction. Remember the fact the value is in the specific register is specified by the ABI, not due to the presence of the 'returned' attribute. 
> 
> That's a great point, this matters when it will be returned in a subreg or with structure return, etc. Thus far we've only been talking about using it for pointers where it would be in a single GPR, but you're absolutely correct in the general case.
> 
> Okay. The questions that remain in my mind are: should we have a step in FunctionAttrs to deduce 'returned'? Are there any other IR-level optimizations we can apply when we see a returned attribute? If somebody asks for a code review of a patch which adds a pass that turns internal-linkage void returning functions into argument returning, just for the benefit of adding the 'returned' attribute, what's the answer?

IMO it would depend on the motivating use cases for the transform. In isolation, it adds no value and so would be rejected, but I suppose I can see cases where that might be interesting. It'd be up to the author to prove benefit, though. Is there something philosophically objectionable to you about such a transform?

Jim

> 
> It's the lack of a good answer to the last question that bothers me the most, but I'm no longer so troubled by the design.
> 
> Nick
> 
>> 
>> Evan
>> 
>> 
>> 
>>> 
>>> You wanted to bring the discussion back to DAE. My problem with the DAE patch is that we're marking as used a value which is not used. That's strange, and I went digging. I don't care much about the change to DAE, but I do have a problem with the 'returned' attribute as defined. That said, my standard for vetoing a patch is very high: I need to know of a better way to do it. I don't have that here. If Dan is convinced that the DAE patch is good to go, and I don't have a stroke of genius about the returned attribute, I'll be happy to see it land.
>>> 
>>> Nick 
>>>> 
>>>>> Making the necessary information available at IR time to let IR passes do register allocation *well* would be a major effort, but not a change to the architecture. At that point, it would just be incremental improvement.
>>>>> 
>>>>>> I agree there are opportunities to further expand this optimization. But I'm not sure that's the relevant discussion here. The question is whether the approach for the most common case (which is important for C++ and x86, arm calling conventions).
>>>>>> 
>>>>>> >
>>>>>> > We don't have the necessary framework to decide when or where 'returned' belongs. We can't know whether it will require expensive extra copies in the callee or if the caller will be under high register pressure, not at the IR level. Heuristics would be an approximation of codegen, with a tradeoff in complexity vs. accuracy  equal to how much of codegen we want to reimplement.
>>>>>> >
>>>>>> > I do understand how it solves your problem in the cross-TU case, where you have this great ABI guarantee that the argument gets returned, which you can use in the register allocator. It isn't even a calling-convention on its own, it's more of a modifier that can be applied to any calling convention. I can see how it ended up as a parameter attribute, much like zeroext is a ABI-level parameter attribute. But once we start to apply its semantics in the same-TU case, it becomes an llvm IR codified hack to work around the lack of interprocedural register allocation. That's not okay.
>>>>>> 
>>>>>> We shouldn't require knowledge of the target and the calling conventions to perform optimization at the IR level. I believe the important thing here is for DAE to be consistent. It should either keep the argument and the "returned" attribute or remove the argument as well as the attribute. The rest of the work should be left to codegen. In today's implementations, the frontend is responsible for some of the ABI lowering. It has detailed knowledge about target calling conventions. It's a reasonable and obvious place to decide on where to place the "returned" attribute.
>>>>> 
>>>>> Yep, I agree with the meat of this. In another part of this thread Dan is examining the implications of how the returned attribute and DAE interact, I hear there's a subtlety involving tail calls? But I absolutely agree the frontend is responsible (and a sensible place) for doing ABI lowering, that it's a good place to decide where 'returned' goes, that we don't want the IR to know things about targets or calling conventions, etc.
>>>>> 
>>>>>> >
>>>>>> > Anyhow, as I said I'm not going to block this patch because I don't have ideas on what would be a better way to do it, but please get an OK from Dan first.
>>>>>> 
>>>>>> This is a long thread so I didn't read all of it. If I understand correctly, Dan's proposal is to change the function prototype to return void and recognize the optimization opportunity at codegen. At the codegen level, the handling of function arguments and return values are scattered all over the place. It's going to add more complexity and I really do not see how it would make the optimization more powerful.
>>>>> 
>>>>> I'm not concerned about how powerful the optimization is. Nobody has offered any performance numbers for this optimization yet. What I'm concerned about is the semantics of the attribute, and the impact on the IR transforms.
>>>>> 
>>>>> One thing I'm considering is whether we've made 'returned' too powerful by making it too general. Again, I'm still stuck on constructive suggestions. I wish I could say "here's how to do it properly", but I haven't got that.
>>>>> 
>>>>> Having Stephen's patch go in is actually fine with me, as long as we understand that we can't allow 'returned' to become a sanctioned place for doing register allocation on the IR (or conversely, that we form a consensus that doing IPA-RA on the IR is the right thing to do -- though I think it isn't). Ideally, I'd like 'returned' to be defined in such a way that it isn't possible to misuse like that. That's my goal with this thread.
>>>>> 
>>>>> Nick
> 
> _______________________________________________
> 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/20130627/135f26a8/attachment.html>