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

[Nick Lewycky]

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?

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
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130627/5074b10d/attachment.html>