New Iteration of subregister liveness patches

[Quentin Colombet]

On Dec 9, 2014, at 5:32 PM, Matthias Braun <matze at braunis.de> wrote:

> Ok,
> 
> I just commited the changes to svn trunk. Thanks everyone, esp. Renato and Quentin, for the detailed reviews!
> 
> SubRegisterLiveness is available as a subtarget switch now, but not enabled by any of the targets yet, as most of them need some additional fixing/testcase changes which I'll address in subsequent commits.
> 
> For the record: I did fix many but not all issues mentioned in the reviews. The following is still missing:
> 
> - Manually creating a single linked list in the LiveInterval class. The alternatives to this are:
>    a) Using the double linked list provided by llvm.
>    b) Turning the manual single linked list into a full featued ADT in llvm.
>  Using a double linked list where a single linked list is wasteful, so I'm against a). Turning the thing into a full-blown ADT template class seemed like too much for a single user in the whole llvm codebase, but if others would prefer that over the current code I will do that.

Fair enough.

> - Sharing the LiveInterval::verify code with the MachineVerifier code. This is not as easy as it looks as you need a working MachineVerifier instance if you want to use the MachineVerifier elsewhere. Additionally that problem isn't introduced by my commits but already existed before... I may take a look into how to make the MachineVerifier more flexible to allow this, but this will be another patch series.

Please file a PR for that.

> - I clarified the comment in the updateLiveIns() function to explain the LiveOutData parameter, but did not add explanation to the other LiveOutData parameters yet. It seems like a better explanation at the point where the LiveOutData class is defined is the way to go instead of explaining the intention over and over again for each function having such a parameter. Anyway this is still TODO

SGTM.

> - The code duplication at places that match uses/defs to subranges and split the subranges on demand is still duplicated at several places in the register coalescer and I think even at other places. This is definitely TODO although not easy as all the appearances vary slightly.

Agree. Maybe file a PR for that one too.

Thanks Matthias,
-Quentin

> 
> Greetings
>    Matthias
> 
>> On Dec 5, 2014, at 1:23 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>> 
>> 
>>> On Dec 5, 2014, at 1:20 PM, Renato Golin <renato.golin at linaro.org> wrote:
>>> 
>>> On 5 December 2014 at 19:19, Tom Stellard <tom at stellard.net> wrote:
>>>> I tested these patches a few months ago and there were some regressions
>>>> on R600.  I'm not sure when I'll get a chance to test them again, but I
>>>> am in favor of merging these patches as soon as possible, since sub-reg
>>>> liveness is hidden behind a subtarget flag and it's easier to fully test
>>>> once it is in ToT.
>>> 
>>> If this is not the default behaviour, and it can be turned on by just
>>> adding a flag, then I don't see why not, after addressing Quentin's
>>> comments. I agree it'll be much easier to test this once it lands.
>> 
>> Same here. And I would add, my concerns can be addressed by subsequent patches.
>> 
>> Cheers,
>> Q.
>> 
>>> 
>>> cheers,
>>> --renato
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>