* [PATCH] cprop_hardreg: fix ORIGINAL_REGNO/REG_ATTRS/REG_POINTER handling
@ 2023-06-22 11:11 Philipp Tomsich
2023-06-23 4:08 ` Andrew Pinski
2023-06-25 13:09 ` Jeff Law
0 siblings, 2 replies; 4+ messages in thread
From: Philipp Tomsich @ 2023-06-22 11:11 UTC (permalink / raw)
To: gcc-patches
Cc: Thiago Jung Bauermann, Tamar Christina, Jakub Jelinek,
Richard Biener, Sergei Trofimovich, Andrew Pinski, Tobian Burnus,
Jeff Law, Kito Cheng, Manolis Tsamis, Philipp Tomsich
From: Manolis Tsamis <manolis.tsamis@vrull.eu>
Fixes: 6a2e8dcbbd4bab3
Propagation for the stack pointer in regcprop was enabled in
6a2e8dcbbd4bab3, but set ORIGINAL_REGNO/REG_ATTRS/REG_POINTER for
stack_pointer_rtx which caused regression (e.g., PR 110313, PR 110308).
This fix adds special handling for stack_pointer_rtx in the places
where maybe_mode_change is called. This also adds an check in
maybe_mode_change to return the stack pointer only when the requested
mode matches the mode of stack_pointer_rtx.
PR 110308
gcc/ChangeLog:
* regcprop.cc (maybe_mode_change): Check stack_pointer_rtx mode.
(find_oldest_value_reg): Special handling of stack_pointer_rtx.
(copyprop_hardreg_forward_1): Ditto.
gcc/testsuite/ChangeLog:
* g++.dg/torture/pr110308.C: New test.
Signed-off-by: Manolis Tsamis <manolis.tsamis@vrull.eu>
Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
---
This addresses both the PRs (110308 and 110313) and was confirmed to
resolve the AArch64 bootstrap issue reported by Thiago.
OK for trunk?
gcc/regcprop.cc | 43 +++++++++++++++++--------
gcc/testsuite/g++.dg/torture/pr110308.C | 30 +++++++++++++++++
2 files changed, 60 insertions(+), 13 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/torture/pr110308.C
diff --git a/gcc/regcprop.cc b/gcc/regcprop.cc
index 6cbfadb181f..fe75b7f1fa0 100644
--- a/gcc/regcprop.cc
+++ b/gcc/regcprop.cc
@@ -423,7 +423,7 @@ maybe_mode_change (machine_mode orig_mode, machine_mode copy_mode,
It's unclear if we need to do the same for other special registers. */
if (regno == STACK_POINTER_REGNUM)
{
- if (orig_mode == new_mode)
+ if (orig_mode == new_mode && new_mode == GET_MODE (stack_pointer_rtx))
return stack_pointer_rtx;
else
return NULL_RTX;
@@ -487,9 +487,14 @@ find_oldest_value_reg (enum reg_class cl, rtx reg, struct value_data *vd)
new_rtx = maybe_mode_change (oldmode, vd->e[regno].mode, mode, i, regno);
if (new_rtx)
{
- ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (reg);
- REG_ATTRS (new_rtx) = REG_ATTRS (reg);
- REG_POINTER (new_rtx) = REG_POINTER (reg);
+ if (new_rtx != stack_pointer_rtx)
+ {
+ ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (reg);
+ REG_ATTRS (new_rtx) = REG_ATTRS (reg);
+ REG_POINTER (new_rtx) = REG_POINTER (reg);
+ }
+ else if (REG_POINTER (new_rtx) != REG_POINTER (reg))
+ return NULL_RTX;
return new_rtx;
}
}
@@ -965,15 +970,27 @@ copyprop_hardreg_forward_1 (basic_block bb, struct value_data *vd)
if (validate_change (insn, &SET_SRC (set), new_rtx, 0))
{
- ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (src);
- REG_ATTRS (new_rtx) = REG_ATTRS (src);
- REG_POINTER (new_rtx) = REG_POINTER (src);
- if (dump_file)
- fprintf (dump_file,
- "insn %u: replaced reg %u with %u\n",
- INSN_UID (insn), regno, REGNO (new_rtx));
- changed = true;
- goto did_replacement;
+ bool can_change;
+ if (new_rtx != stack_pointer_rtx)
+ {
+ ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (src);
+ REG_ATTRS (new_rtx) = REG_ATTRS (src);
+ REG_POINTER (new_rtx) = REG_POINTER (src);
+ can_change = true;
+ }
+ else
+ can_change
+ = (REG_POINTER (new_rtx) == REG_POINTER (src));
+
+ if (can_change)
+ {
+ if (dump_file)
+ fprintf (dump_file,
+ "insn %u: replaced reg %u with %u\n",
+ INSN_UID (insn), regno, REGNO (new_rtx));
+ changed = true;
+ goto did_replacement;
+ }
}
/* We need to re-extract as validate_change clobbers
recog_data. */
diff --git a/gcc/testsuite/g++.dg/torture/pr110308.C b/gcc/testsuite/g++.dg/torture/pr110308.C
new file mode 100644
index 00000000000..ddd30d4fc3f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr110308.C
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-g2 -O2" } */
+
+int channelCount, decodeBlock_outputLength;
+struct BlockCodec {
+ virtual int decodeBlock(const unsigned char *, short *);
+};
+struct ms_adpcm_state {
+ char predictorIndex;
+ int sample1;
+ ms_adpcm_state();
+};
+bool decodeBlock_ok;
+void encodeBlock() { ms_adpcm_state(); }
+struct MSADPCM : BlockCodec {
+ int decodeBlock(const unsigned char *, short *);
+};
+void decodeSample(ms_adpcm_state, bool *);
+int MSADPCM::decodeBlock(const unsigned char *, short *) {
+ ms_adpcm_state decoderState[2];
+ ms_adpcm_state *state[2];
+ state[0] = &decoderState[0];
+ if (channelCount == 2)
+ state[1] = &decoderState[0];
+ short m_coefficients[state[1]->predictorIndex];
+ for (int i = 0; i < channelCount; i++)
+ ++state[i]->sample1;
+ decodeSample(*state[1], &decodeBlock_ok);
+ return decodeBlock_outputLength;
+}
\ No newline at end of file
--
2.34.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cprop_hardreg: fix ORIGINAL_REGNO/REG_ATTRS/REG_POINTER handling
2023-06-22 11:11 [PATCH] cprop_hardreg: fix ORIGINAL_REGNO/REG_ATTRS/REG_POINTER handling Philipp Tomsich
@ 2023-06-23 4:08 ` Andrew Pinski
2023-06-25 13:09 ` Jeff Law
1 sibling, 0 replies; 4+ messages in thread
From: Andrew Pinski @ 2023-06-23 4:08 UTC (permalink / raw)
To: Philipp Tomsich
Cc: gcc-patches, Thiago Jung Bauermann, Tamar Christina,
Jakub Jelinek, Richard Biener, Sergei Trofimovich, Tobian Burnus,
Jeff Law, Kito Cheng, Manolis Tsamis
On Thu, Jun 22, 2023 at 4:12 AM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> From: Manolis Tsamis <manolis.tsamis@vrull.eu>
>
> Fixes: 6a2e8dcbbd4bab3
>
> Propagation for the stack pointer in regcprop was enabled in
> 6a2e8dcbbd4bab3, but set ORIGINAL_REGNO/REG_ATTRS/REG_POINTER for
> stack_pointer_rtx which caused regression (e.g., PR 110313, PR 110308).
>
> This fix adds special handling for stack_pointer_rtx in the places
> where maybe_mode_change is called. This also adds an check in
> maybe_mode_change to return the stack pointer only when the requested
> mode matches the mode of stack_pointer_rtx.
The only comment I have is on the testcase.
>
> PR 110308
>
> gcc/ChangeLog:
>
> * regcprop.cc (maybe_mode_change): Check stack_pointer_rtx mode.
> (find_oldest_value_reg): Special handling of stack_pointer_rtx.
> (copyprop_hardreg_forward_1): Ditto.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/torture/pr110308.C: New test.
>
> Signed-off-by: Manolis Tsamis <manolis.tsamis@vrull.eu>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
>
> ---
> This addresses both the PRs (110308 and 110313) and was confirmed to
> resolve the AArch64 bootstrap issue reported by Thiago.
>
> OK for trunk?
>
> gcc/regcprop.cc | 43 +++++++++++++++++--------
> gcc/testsuite/g++.dg/torture/pr110308.C | 30 +++++++++++++++++
> 2 files changed, 60 insertions(+), 13 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/torture/pr110308.C
>
> diff --git a/gcc/regcprop.cc b/gcc/regcprop.cc
> index 6cbfadb181f..fe75b7f1fa0 100644
> --- a/gcc/regcprop.cc
> +++ b/gcc/regcprop.cc
> @@ -423,7 +423,7 @@ maybe_mode_change (machine_mode orig_mode, machine_mode copy_mode,
> It's unclear if we need to do the same for other special registers. */
> if (regno == STACK_POINTER_REGNUM)
> {
> - if (orig_mode == new_mode)
> + if (orig_mode == new_mode && new_mode == GET_MODE (stack_pointer_rtx))
> return stack_pointer_rtx;
> else
> return NULL_RTX;
> @@ -487,9 +487,14 @@ find_oldest_value_reg (enum reg_class cl, rtx reg, struct value_data *vd)
> new_rtx = maybe_mode_change (oldmode, vd->e[regno].mode, mode, i, regno);
> if (new_rtx)
> {
> - ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (reg);
> - REG_ATTRS (new_rtx) = REG_ATTRS (reg);
> - REG_POINTER (new_rtx) = REG_POINTER (reg);
> + if (new_rtx != stack_pointer_rtx)
> + {
> + ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (reg);
> + REG_ATTRS (new_rtx) = REG_ATTRS (reg);
> + REG_POINTER (new_rtx) = REG_POINTER (reg);
> + }
> + else if (REG_POINTER (new_rtx) != REG_POINTER (reg))
> + return NULL_RTX;
> return new_rtx;
> }
> }
> @@ -965,15 +970,27 @@ copyprop_hardreg_forward_1 (basic_block bb, struct value_data *vd)
>
> if (validate_change (insn, &SET_SRC (set), new_rtx, 0))
> {
> - ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (src);
> - REG_ATTRS (new_rtx) = REG_ATTRS (src);
> - REG_POINTER (new_rtx) = REG_POINTER (src);
> - if (dump_file)
> - fprintf (dump_file,
> - "insn %u: replaced reg %u with %u\n",
> - INSN_UID (insn), regno, REGNO (new_rtx));
> - changed = true;
> - goto did_replacement;
> + bool can_change;
> + if (new_rtx != stack_pointer_rtx)
> + {
> + ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (src);
> + REG_ATTRS (new_rtx) = REG_ATTRS (src);
> + REG_POINTER (new_rtx) = REG_POINTER (src);
> + can_change = true;
> + }
> + else
> + can_change
> + = (REG_POINTER (new_rtx) == REG_POINTER (src));
> +
> + if (can_change)
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + "insn %u: replaced reg %u with %u\n",
> + INSN_UID (insn), regno, REGNO (new_rtx));
> + changed = true;
> + goto did_replacement;
> + }
> }
> /* We need to re-extract as validate_change clobbers
> recog_data. */
> diff --git a/gcc/testsuite/g++.dg/torture/pr110308.C b/gcc/testsuite/g++.dg/torture/pr110308.C
> new file mode 100644
> index 00000000000..ddd30d4fc3f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/pr110308.C
> @@ -0,0 +1,30 @@
> +/* { dg-do compile } */
> +/* { dg-options "-g2 -O2" } */
Since this is in `g++.dg/torture`, you don't need dg-options as the
DG_TORTURE_OPTIONS includes `-O3 -g` already (note `-g` is the same as
`-g2`).
Thanks,
Andrew
> +
> +int channelCount, decodeBlock_outputLength;
> +struct BlockCodec {
> + virtual int decodeBlock(const unsigned char *, short *);
> +};
> +struct ms_adpcm_state {
> + char predictorIndex;
> + int sample1;
> + ms_adpcm_state();
> +};
> +bool decodeBlock_ok;
> +void encodeBlock() { ms_adpcm_state(); }
> +struct MSADPCM : BlockCodec {
> + int decodeBlock(const unsigned char *, short *);
> +};
> +void decodeSample(ms_adpcm_state, bool *);
> +int MSADPCM::decodeBlock(const unsigned char *, short *) {
> + ms_adpcm_state decoderState[2];
> + ms_adpcm_state *state[2];
> + state[0] = &decoderState[0];
> + if (channelCount == 2)
> + state[1] = &decoderState[0];
> + short m_coefficients[state[1]->predictorIndex];
> + for (int i = 0; i < channelCount; i++)
> + ++state[i]->sample1;
> + decodeSample(*state[1], &decodeBlock_ok);
> + return decodeBlock_outputLength;
> +}
> \ No newline at end of file
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cprop_hardreg: fix ORIGINAL_REGNO/REG_ATTRS/REG_POINTER handling
2023-06-22 11:11 [PATCH] cprop_hardreg: fix ORIGINAL_REGNO/REG_ATTRS/REG_POINTER handling Philipp Tomsich
2023-06-23 4:08 ` Andrew Pinski
@ 2023-06-25 13:09 ` Jeff Law
2023-06-28 14:10 ` Philipp Tomsich
1 sibling, 1 reply; 4+ messages in thread
From: Jeff Law @ 2023-06-25 13:09 UTC (permalink / raw)
To: Philipp Tomsich, gcc-patches
Cc: Thiago Jung Bauermann, Tamar Christina, Jakub Jelinek,
Richard Biener, Sergei Trofimovich, Andrew Pinski, Tobian Burnus,
Jeff Law, Kito Cheng, Manolis Tsamis
On 6/22/23 05:11, Philipp Tomsich wrote:
> From: Manolis Tsamis <manolis.tsamis@vrull.eu>
>
> Fixes: 6a2e8dcbbd4bab3
>
> Propagation for the stack pointer in regcprop was enabled in
> 6a2e8dcbbd4bab3, but set ORIGINAL_REGNO/REG_ATTRS/REG_POINTER for
> stack_pointer_rtx which caused regression (e.g., PR 110313, PR 110308).
>
> This fix adds special handling for stack_pointer_rtx in the places
> where maybe_mode_change is called. This also adds an check in
> maybe_mode_change to return the stack pointer only when the requested
> mode matches the mode of stack_pointer_rtx.
>
> PR 110308
Should be
PR debug/110308
>
> gcc/ChangeLog:
>
> * regcprop.cc (maybe_mode_change): Check stack_pointer_rtx mode.
> (find_oldest_value_reg): Special handling of stack_pointer_rtx.
> (copyprop_hardreg_forward_1): Ditto.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/torture/pr110308.C: New test.
I don't doubt the need for the special handling of the stack pointer,
but it's not obvious why it's needed. So my request is that both humks
which specialize handling of ORIGINAL_REGNO, REG_ATTRS & REG_POINTER
have a comment indicating why we must not adjust those values when
NEW_RTX is STACK_POINTER_RTX.
OK with that change.
Jeff
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cprop_hardreg: fix ORIGINAL_REGNO/REG_ATTRS/REG_POINTER handling
2023-06-25 13:09 ` Jeff Law
@ 2023-06-28 14:10 ` Philipp Tomsich
0 siblings, 0 replies; 4+ messages in thread
From: Philipp Tomsich @ 2023-06-28 14:10 UTC (permalink / raw)
To: Jeff Law
Cc: gcc-patches, Thiago Jung Bauermann, Tamar Christina,
Jakub Jelinek, Richard Biener, Sergei Trofimovich, Andrew Pinski,
Tobian Burnus, Jeff Law, Kito Cheng, Manolis Tsamis
Thanks! Applied to master with the requested changes as
417b8379b32945d61f1ce3d8281bee063eea1937.
Note that the final version factors out the duplicated logic, so we
now have a single place to add the comments.
Philipp.
On Sun, 25 Jun 2023 at 06:09, Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 6/22/23 05:11, Philipp Tomsich wrote:
> > From: Manolis Tsamis <manolis.tsamis@vrull.eu>
> >
> > Fixes: 6a2e8dcbbd4bab3
> >
> > Propagation for the stack pointer in regcprop was enabled in
> > 6a2e8dcbbd4bab3, but set ORIGINAL_REGNO/REG_ATTRS/REG_POINTER for
> > stack_pointer_rtx which caused regression (e.g., PR 110313, PR 110308).
> >
> > This fix adds special handling for stack_pointer_rtx in the places
> > where maybe_mode_change is called. This also adds an check in
> > maybe_mode_change to return the stack pointer only when the requested
> > mode matches the mode of stack_pointer_rtx.
> >
> > PR 110308
> Should be
> PR debug/110308
>
>
> >
> > gcc/ChangeLog:
> >
> > * regcprop.cc (maybe_mode_change): Check stack_pointer_rtx mode.
> > (find_oldest_value_reg): Special handling of stack_pointer_rtx.
> > (copyprop_hardreg_forward_1): Ditto.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/torture/pr110308.C: New test.
> I don't doubt the need for the special handling of the stack pointer,
> but it's not obvious why it's needed. So my request is that both humks
> which specialize handling of ORIGINAL_REGNO, REG_ATTRS & REG_POINTER
> have a comment indicating why we must not adjust those values when
> NEW_RTX is STACK_POINTER_RTX.
>
> OK with that change.
>
> Jeff
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-28 14:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-22 11:11 [PATCH] cprop_hardreg: fix ORIGINAL_REGNO/REG_ATTRS/REG_POINTER handling Philipp Tomsich
2023-06-23 4:08 ` Andrew Pinski
2023-06-25 13:09 ` Jeff Law
2023-06-28 14:10 ` Philipp Tomsich
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).