From: Richard Sandiford <richard.sandiford@arm.com>
To: Robin Dapp <rdapp.gcc@gmail.com>
Cc: Jeff Law <jeffreyalaw@gmail.com>, gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] fwprop: Allow UNARY_P and check register pressure.
Date: Tue, 05 Sep 2023 09:38:31 +0100 [thread overview]
Message-ID: <mptpm2xt3p4.fsf@arm.com> (raw)
In-Reply-To: <77daf152-e1fe-4980-8297-d37901d925e8@gmail.com> (Robin Dapp's message of "Tue, 5 Sep 2023 08:53:13 +0200")
Robin Dapp <rdapp.gcc@gmail.com> writes:
>> So I don't think I have a good feel for the advantages and disadvantages
>> of doing this. Robin's analysis of the aarch64 changes was nice and
>> detailed though. I think the one that worries me most is the addressing
>> mode one. fwprop is probably the first chance we get to propagate adds
>> into addresses, and virtual register elimination means that some of
>> those opportunities won't show up in gimple.
>>
>> There again, virtual register elimination wouldn't be the reason for
>> the ld4_s8.c failure. Perhaps there's something missing in expand.
>>
>> Other than that, I think my main question is: why just unary operations?
>> Is the underlying assumption that we only want to propagate a maximum of
>> one register? If so, then I think we should check for that directly, by
>> iterating over subrtxes.
>
> The main reason for stopping at unary operations was to limit the scope
> and change as little as possible (not restricting the change to one
> register). I'm currently testing a v2 that iterates over subrtxs.
Thanks. Definitely no problem with doing things in small steps, but IMO
it's better if each choice of step can still be justified in its own terms.
>> Perhaps we should allow the optimisation without register-pressure
>> information if (a) the source register and destination register are
>> in the same pressure class and (b) all uses of the destination are
>> being replaced. (FWIW, rtl-ssa should make it easier to try to
>> replace all definitions at once, with an all-or-nothing choice,
>> if we ever wanted to do that.)
>
> I presume you're referring to replacing one register (dest) in all using
> insns? Source and destination are somewhat overloaded in fwprop context
> because I'm thinking of the "to be replaced" register as dest when it's
> actually the replacement register.
Yeah.
> AFAICT fwprop currently iterates over insns, going through all their uses
> and trying if an individual use can be substituted. Do you suggest to
> change this general iteration order to iterate over the defs of an insn
> and then try to replace all the uses at once (e.g. using ssa->change_insns)?
No, I was just noting in passing that we could try do that if we wanted to.
The current code is a fairly mechanical conversion of the original DF-based
code, but there's no reason why it has to continue to work the way it
does now.
> When keeping the current order, wouldn't we need to store all potential
> changes instead of committing them and later apply them in bulk, e.g.
> grouped by use? This order would also help to pick the propagation
> with the most number of uses (i.e. propagation potential) but maybe
> I'm misunderstanding?
I imagine doing it in reverse postorder would still make sense.
But my point was that, for the current fwprop limitation of substituting
into exactly one use of a register, we can check whether that use is
the *only* use of register.
I.e. if we substitute:
A: (set (reg R1) (foo (reg R2)))
into:
B: (set ... (reg R1) ...)
if R1 and R2 are likely to be in the same register class, and if B
is the only user of R2, then we don't need to calculate register
pressure. The change is either neutral (if R2 died in A) or an
improvement (if R2 doesn't die in A, and so R1 and R2 were previously
live at the same time).
Thanks,
Richard
next prev parent reply other threads:[~2023-09-05 8:38 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-07 10:26 Robin Dapp
2023-08-24 14:06 ` Robin Dapp
2023-08-28 23:33 ` Jeff Law
2023-08-29 11:40 ` Richard Sandiford
2023-09-05 6:53 ` Robin Dapp
2023-09-05 8:38 ` Richard Sandiford [this message]
2023-09-05 8:45 ` Robin Dapp
2023-09-06 11:22 ` Robin Dapp
2023-09-06 20:44 ` Richard Sandiford
2023-09-07 7:56 ` Robin Dapp
2023-09-07 13:42 ` Richard Sandiford
2023-09-07 14:25 ` Robin Dapp
2023-09-26 16:24 ` Richard Sandiford
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mptpm2xt3p4.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jeffreyalaw@gmail.com \
--cc=rdapp.gcc@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).