New Iteration of subregister liveness patches

Matthias Braun matze at braunis.de
Tue Dec 9 17:32:23 PST 2014


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

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





More information about the llvm-commits mailing list