Message ID | 20230622111154.2837175-1-philipp.tomsich@vrull.eu |
---|---|
State | New |
Headers | show |
Series | cprop_hardreg: fix ORIGINAL_REGNO/REG_ATTRS/REG_POINTER handling | expand |
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 >
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
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
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