Re: [PATCH 0/7] ira/lra: Support subreg coalesce

[Jeff Law <jeffreyalaw@gmail.com>]

On 11/8/23 02:40, Richard Sandiford wrote:
> Lehua Ding <lehua.ding@rivai.ai> writes:
>> Hi,
>>
>> These patchs try to support subreg coalesce feature in
>> register allocation passes (ira and lra).
> 
> Thanks a lot for the series.  This is definitely something we've
> needed for a while.
> 
> I probably won't be able to look at it in detail for a couple of weeks
> (and the real review should come from Vlad anyway), but one initial
> comment:
Absolutely agreed on the above.

The other thing to ponder.  Jivan and I have been banging on Joern's 
sub-object tracking bits for a totally different problem in the RISC-V 
space.  But there may be some overlap.

Essentially Joern's code tracks liveness for a few chunks in registers. 
bits 0..7, bits 8..15, bits 16..31 and bits 32..63.  This includes 
propagating liveness from the destination through to the sources.  SO 
for example if we have

(set (reg:SI dest) (plus:SI (srcreg1:SI) (srcreg2:SI)))

If we had previously determined that only bits 0..15 were live in DEST, 
then we'll propagate that into the source registers.

The goal is to ultimately transform something like

(set (dest:mode) (any_extend:mode (reg:narrower_mode)))

into

(set (dest:mode) (subreg:mode (reg:narrower_mode)))

Where the latter typically will get simplified and propagated away.


Joern's code is a bit of a mess, but Jivan and I are slowly untangling 
it from a correctness standpoint.  It'll also need the usual cleanups.

Anyway, point being I think it'll be worth looking at Lehua's bits and 
Joern's bits to see if there's anything that can and should be shared. 
Given I'm getting fairly familiar with Joern's bits, that likely falls 
to me.

Jeff

> 
> Tracking subreg liveness will sometimes expose dead code that
> wasn't obvious without it.  PR89606 has an example of this.
> There the dead code was introduced by init-regs, and there's a
> debate about (a) whether init-regs should still be run and (b) if it
> should still be run, whether it should use subreg liveness tracking too.
> 
> But I think such dead code is possible even without init-regs.
> So for the purpose of this series, I think the init-regs behaviour
> in that PR creates a helpful example.
> 
> I agree with Richi of course that compile-time is a concern.
> The patch seems to add quite a bit of new data to ira_allocno,
> but perhaps that's OK.  ira_object + ira_allocno is already quite big.
> 
> However:
> 
> @@ -387,8 +398,8 @@ struct ira_allocno
>     /* An array of structures describing conflict information and live
>        ranges for each object associated with the allocno.  There may be
>        more than one such object in cases where the allocno represents a
> -     multi-word register.  */
> -  ira_object_t objects[2];
> +     multi-hardreg pesudo.  */
> +  std::vector<ira_object_t> objects;
>     /* Registers clobbered by intersected calls.  */
>      HARD_REG_SET crossed_calls_clobbered_regs;
>     /* Array of usage costs (accumulated and the one updated during
> 
> adds an extra level of indirection (and separate extra storage) for
> every allocno, not just multi-hardreg ones.  It'd be worth optimising
> the data structures' representation of single-hardreg pseudos even if
> that slows down the multi-hardreg code, since single-hardreg pseudos are
> so much more common.  And the different single-hardreg and multi-hardreg
> representations could be hidden behind accessors, to make life easier
> for consumers.  (Of course, performance of the accessors is also then
> an issue. :))
> 
> Richard