On 11/8/23 02:40, Richard Sandiford wrote:
Lehua Ding <lehua.d...@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
