From: Richard Sandiford <richard.sandiford@arm.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Hongtao Liu <crazylht@gmail.com>,
Eric Botcazou <ebotcazou@libertysurf.fr>,
Jeff Law <jeffreyalaw@gmail.com>,
gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] regcprop: Fix another cprop_hardreg bug [PR100342]
Date: Tue, 11 May 2021 11:59:24 +0100 [thread overview]
Message-ID: <mptcztx359v.fsf@arm.com> (raw)
In-Reply-To: <20210505174446.GU1179226@tucnak> (Jakub Jelinek's message of "Wed, 5 May 2021 19:44:46 +0200")
Jakub Jelinek <jakub@redhat.com> writes:
> On Tue, Jan 19, 2021 at 04:10:33PM +0000, Richard Sandiford via Gcc-patches wrote:
>> Ah, ok, thanks for the extra context.
>>
>> So AIUI the problem when recording xmm2<-di isn't just:
>>
>> [A] partial_subreg_p (vd->e[sr].mode, GET_MODE (src))
>>
>> but also that:
>>
>> [B] partial_subreg_p (vd->e[sr].mode, vd->e[vd->e[sr].oldest_regno].mode)
>>
>> For example, all registers in this sequence can be part of the same chain:
>>
>> (set (reg:HI R1) (reg:HI R0))
>> (set (reg:SI R2) (reg:SI R1)) // [A]
>> (set (reg:DI R3) (reg:DI R2)) // [A]
>> (set (reg:SI R4) (reg:SI R[0-3]))
>> (set (reg:HI R5) (reg:HI R[0-4]))
>>
>> But:
>>
>> (set (reg:SI R1) (reg:SI R0))
>> (set (reg:HI R2) (reg:HI R1))
>> (set (reg:SI R3) (reg:SI R2)) // [A] && [B]
>>
>> is problematic because it dips below the precision of the oldest regno
>> and then increases again.
>>
>> When this happens, I guess we have two choices:
>>
>> (1) what the patch does: treat R3 as the start of a new chain.
>> (2) pretend that the copy occured in vd->e[sr].mode instead
>> (i.e. copy vd->e[sr].mode to vd->e[dr].mode)
>>
>> I guess (2) would need to be subject to REG_CAN_CHANGE_MODE_P.
>> Maybe the optimisation provided by (2) compared to (1) isn't common
>> enough to be worth the complication.
>>
>> I think we should test [B] as well as [A] though. The pass is set
>> up to do some quite elaborate mode changes and I think rejecting
>> [A] on its own would make some of the other code redundant.
>> It also feels like it should be a seperate “if” or “else if”,
>> with its own comment.
>
> Unfortunately, we now have a testcase that shows that testing also [B]
> is a problem (unfortunately now latent on the trunk, only reproduces
> on 10 and 11 branches).
This whole area feels way more complicated than it ought to be :-/
> The comment in the patch tries to list just the interesting instructions,
> we have a 64-bit value, copy low 8 bit of those to another register,
> copy full 64 bits to another register and then clobber the original register.
> Before that (set (reg:DI r14) (const_int ...)) we have a chain
> DI r14, QI si, DI bp , that instruction drops the DI r14 from that chain, so
> we have QI si, DI bp , si being the oldest_regno.
> Next DI si is copied into DI dx. Only the low 8 bits of that are defined,
> the rest is unspecified, but we would add DI dx into that same chain at the
> end, so QI si, DI bp, DI dx [*]. Next si is overwritten, so the chain is
> DI bp, DI dx. And then we see (set (reg:DI dx) (reg:DI bp)) and remove it
> as redundant, because we think bp and dx are already equivalent, when in
> reality that is true only for the lowpart 8 bits.
> I believe the [*] marked step above is where the bug is.
>
> The committed regcprop.c (copy_value) change (but only committed to
> trunk/11, not to 10) added
> else if (partial_subreg_p (vd->e[sr].mode, GET_MODE (src))
> && partial_subreg_p (vd->e[sr].mode,
> vd->e[vd->e[sr].oldest_regno].mode))
> return;
> and while the first partial_subreg_p call returns true, the second one
> doesn't; before the (set (reg:DI r14) (const_int ...)) insn it would be
> true and we'd return, but as that reg got clobbered, si became the oldest
> regno in the chain and so vd->e[vd->e[sr].oldest_regno].mode is QImode
> and vd->e[sr].mode is QImode too, so the second partial_subreg_p is false.
> But as the testcase shows, what is the oldest_regno in the chain is
> something that changes over time, so relying on it for anything is
> problematic, something could have a different oldest_regno and later
> on get a different oldest_regno (perhaps with different mode) because
> the oldest_regno got overwritten and it can change both ways.
>
> I wrote the following patch (originally against 10 branch because that is
> where Uros has been debugging it) and bootstrapped/regtested it on 11
> branch successfully.
> It effectively implements your (2) above; I'm not sure if
> REG_CAN_CHANGE_MODE_P is needed there, because it is already tested in
> find_oldest_value_reg -> maybe_mode_change -> mode_change_ok.
The REG_CAN_CHANGE_MODE_P test would in this case be for
vd->e[dr].mode → vd->e[sr].mode, rather than oldest_regno's mode.
I'm just worried that:
(set (reg:HI R1) (reg:HI R0))
(set (reg:SI R2) (reg:SI R1))
isn't equivalent to:
(set (reg:HI R1) (reg:HI R0))
(set (reg:HI R2) (reg:HI R1))
if REG_CAN_CHANGE_MODE_P is false for either the R2 or R1 change.
If we pretend that it is when building the chain then there's a
risk of GIGO when using it in find_oldest_value_reg.
(Although in this case SI and HI are both valid for R1,
REG_CAN_CHANGE_MODE_P might still be false if the HI bits are
not in the low 16 bits of the SI. That's unlikely in this case,
but a similar thing can happen for vector modes or multi-register modes.)
I'm not saying the patch is wrong. I just wanted to clarify
why I thought the check might be needed.
Thanks,
Richard
next prev parent reply other threads:[~2021-05-11 10:59 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-18 9:16 [PATCH] [PR rtl/optimization/98694] Fix incorrect optimization by cprop_hardreg Hongtao Liu
2021-01-18 10:18 ` Richard Sandiford
2021-01-18 10:43 ` Hongtao Liu
2021-01-18 10:51 ` Hongtao Liu
2021-01-18 11:10 ` Richard Sandiford
2021-01-19 0:59 ` Hongtao Liu
2021-01-19 12:38 ` Richard Sandiford
2021-01-19 14:45 ` Jakub Jelinek
2021-01-19 16:10 ` Richard Sandiford
2021-01-20 4:35 ` Hongtao Liu
2021-01-20 4:40 ` Hongtao Liu
2021-01-20 12:56 ` H.J. Lu
2021-01-20 14:14 ` Richard Sandiford
2021-01-21 5:25 ` Hongtao Liu
2021-05-05 17:44 ` [PATCH] regcprop: Fix another cprop_hardreg bug [PR100342] Jakub Jelinek
2021-05-06 8:50 ` Jakub Jelinek
2021-05-11 10:59 ` Richard Sandiford [this message]
2021-05-13 15:37 ` Jakub Jelinek
2021-05-13 17:01 ` Jakub Jelinek
2021-05-14 9:09 ` 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=mptcztx359v.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=crazylht@gmail.com \
--cc=ebotcazou@libertysurf.fr \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=jeffreyalaw@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).