| Message ID | 4fce730c-4282-434a-9088-794385e332de@gjlay.de |
|---|---|
| State | New |
| Headers | show |
| Series | [reload] PR116326: Add #define IN_RELOAD1_CC | expand |
On 9/6/24 3:40 AM, Georg-Johann Lay wrote: > The reason for PR116326 is that LRA and reload require different > ELIMINABLE_REGS for a multi-register frame pointer. As ELIMINABLE_REGS > is used to initialize static const objects, it is not possible to make > ELIMINABLE_REGS to depend on options or patch it in some target hook. > > It was also concluded that it is not desirable to adjust reload so that > it behaves like LRA, but a hack like #define IN_RELOAD1_CC at the top > of reload1.cc would be fine, see > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116326#c8 > > This is an according patch that defines IN_RELOAD1_CC and uses it in > avr.h to define ELIMINABLE_REGS. > > This is only required for trunk. > > PR116326 occurred for some test case in avr-torture.exp, so I didn't > duplicate the test case. > > As it appears, this patch also fixes: > > https://gcc.gnu.org/PR116324 > https://gcc.gnu.org/PR116325 > https://gcc.gnu.org/PR116550 > > Johann > > -- > > AVR: target/116326 - Adjust ELIMINABLE_REGS to reload resp. LRA. > > PR target/116326 > gcc/ > * reload1.cc (IN_RELOAD1_CC): Define prior to all includes. > * config/avr/avr.h (ELIMINABLE_REGS): Depend on IN_RELOAD1_CC. I'm going to have to NAK this. It's too hackish, even with reload going away.... Jeff
Am 07.09.24 um 18:35 schrieb Jeff Law: > On 9/6/24 3:40 AM, Georg-Johann Lay wrote: >> The reason for PR116326 is that LRA and reload require different >> ELIMINABLE_REGS for a multi-register frame pointer. As ELIMINABLE_REGS >> is used to initialize static const objects, it is not possible to make >> ELIMINABLE_REGS to depend on options or patch it in some target hook. >> >> It was also concluded that it is not desirable to adjust reload so that >> it behaves like LRA, but a hack like #define IN_RELOAD1_CC at the top >> of reload1.cc would be fine, see >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116326#c8 >> >> This is an according patch that defines IN_RELOAD1_CC and uses it in >> avr.h to define ELIMINABLE_REGS. >> >> This is only required for trunk. >> >> PR116326 occurred for some test case in avr-torture.exp, so I didn't >> duplicate the test case. >> >> As it appears, this patch also fixes: >> >> https://gcc.gnu.org/PR116324 >> https://gcc.gnu.org/PR116325 >> https://gcc.gnu.org/PR116550 >> >> Johann >> >> -- >> >> AVR: target/116326 - Adjust ELIMINABLE_REGS to reload resp. LRA. >> >> PR target/116326 >> gcc/ >> * reload1.cc (IN_RELOAD1_CC): Define prior to all includes. >> * config/avr/avr.h (ELIMINABLE_REGS): Depend on IN_RELOAD1_CC. > I'm going to have to NAK this. It's too hackish, even with reload going > away.... > > Jeff So what is your proposal? I'd agree with Richard that we don't want to change reload implementations such that they agree on ELIMINABLE_REGS. A different way would be to change reload1.cc such that it includes reload.h prior to tm.h, i.e. prior to backend.h. This would make the hack avr realm entirely, but it won't be trivially no-op. Johann
On Sat, Sep 7, 2024 at 10:49 AM Georg-Johann Lay <avr@gjlay.de> wrote: > > Am 07.09.24 um 18:35 schrieb Jeff Law: > > On 9/6/24 3:40 AM, Georg-Johann Lay wrote: > >> The reason for PR116326 is that LRA and reload require different > >> ELIMINABLE_REGS for a multi-register frame pointer. As ELIMINABLE_REGS > >> is used to initialize static const objects, it is not possible to make > >> ELIMINABLE_REGS to depend on options or patch it in some target hook. > >> > >> It was also concluded that it is not desirable to adjust reload so that > >> it behaves like LRA, but a hack like #define IN_RELOAD1_CC at the top > >> of reload1.cc would be fine, see > >> > >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116326#c8 > >> > >> This is an according patch that defines IN_RELOAD1_CC and uses it in > >> avr.h to define ELIMINABLE_REGS. > >> > >> This is only required for trunk. > >> > >> PR116326 occurred for some test case in avr-torture.exp, so I didn't > >> duplicate the test case. > >> > >> As it appears, this patch also fixes: > >> > >> https://gcc.gnu.org/PR116324 > >> https://gcc.gnu.org/PR116325 > >> https://gcc.gnu.org/PR116550 > >> > >> Johann > >> > >> -- > >> > >> AVR: target/116326 - Adjust ELIMINABLE_REGS to reload resp. LRA. > >> > >> PR target/116326 > >> gcc/ > >> * reload1.cc (IN_RELOAD1_CC): Define prior to all includes. > >> * config/avr/avr.h (ELIMINABLE_REGS): Depend on IN_RELOAD1_CC. > > I'm going to have to NAK this. It's too hackish, even with reload going > > away.... > > > > Jeff > > So what is your proposal? > > I'd agree with Richard that we don't want to change reload > implementations such that they agree on ELIMINABLE_REGS. > > A different way would be to change reload1.cc such that it includes > reload.h prior to tm.h, i.e. prior to backend.h. This would make the > hack avr realm entirely, but it won't be trivially no-op. > > Johann Why not add RELOAD_ELIMINABLE_REGS and default it to ELIMINABLE_REGS?
Am 07.09.24 um 19:51 schrieb H.J. Lu: > On Sat, Sep 7, 2024 at 10:49 AM Georg-Johann Lay <avr@gjlay.de> wrote: >> Am 07.09.24 um 18:35 schrieb Jeff Law: >>> On 9/6/24 3:40 AM, Georg-Johann Lay wrote: >>>> The reason for PR116326 is that LRA and reload require different >>>> ELIMINABLE_REGS for a multi-register frame pointer. As ELIMINABLE_REGS >>>> is used to initialize static const objects, it is not possible to make >>>> ELIMINABLE_REGS to depend on options or patch it in some target hook. >>>> >>>> It was also concluded that it is not desirable to adjust reload so that >>>> it behaves like LRA, but a hack like #define IN_RELOAD1_CC at the top >>>> of reload1.cc would be fine, see >>>> >>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116326#c8 >>>> >>>> This is an according patch that defines IN_RELOAD1_CC and uses it in >>>> avr.h to define ELIMINABLE_REGS. >>>> >>>> This is only required for trunk. >>>> >>>> PR116326 occurred for some test case in avr-torture.exp, so I didn't >>>> duplicate the test case. >>>> >>>> As it appears, this patch also fixes: >>>> >>>> https://gcc.gnu.org/PR116324 >>>> https://gcc.gnu.org/PR116325 >>>> https://gcc.gnu.org/PR116550 >>>> >>>> Johann >>>> >>>> -- >>>> >>>> AVR: target/116326 - Adjust ELIMINABLE_REGS to reload resp. LRA. >>>> >>>> PR target/116326 >>>> gcc/ >>>> * reload1.cc (IN_RELOAD1_CC): Define prior to all includes. >>>> * config/avr/avr.h (ELIMINABLE_REGS): Depend on IN_RELOAD1_CC. >>> I'm going to have to NAK this. It's too hackish, even with reload going >>> away.... >>> >>> Jeff >> >> So what is your proposal? >> >> I'd agree with Richard that we don't want to change reload >> implementations such that they agree on ELIMINABLE_REGS. >> >> A different way would be to change reload1.cc such that it includes >> reload.h prior to tm.h, i.e. prior to backend.h. This would make the >> hack avr realm entirely, but it won't be trivially no-op. >> >> Johann > > Why not add RELOAD_ELIMINABLE_REGS and default it > to ELIMINABLE_REGS? I can't find RELOAD_ELIMINABLE_REGS anywhere. So are you proposing a new hook macro? Johann
On Sat, Sep 7, 2024, 11:07 AM Georg-Johann Lay <avr@gjlay.de> wrote: > Am 07.09.24 um 19:51 schrieb H.J. Lu: > > On Sat, Sep 7, 2024 at 10:49 AM Georg-Johann Lay <avr@gjlay.de> wrote: > >> Am 07.09.24 um 18:35 schrieb Jeff Law: > >>> On 9/6/24 3:40 AM, Georg-Johann Lay wrote: > >>>> The reason for PR116326 is that LRA and reload require different > >>>> ELIMINABLE_REGS for a multi-register frame pointer. As > ELIMINABLE_REGS > >>>> is used to initialize static const objects, it is not possible to make > >>>> ELIMINABLE_REGS to depend on options or patch it in some target hook. > >>>> > >>>> It was also concluded that it is not desirable to adjust reload so > that > >>>> it behaves like LRA, but a hack like #define IN_RELOAD1_CC at the top > >>>> of reload1.cc would be fine, see > >>>> > >>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116326#c8 > >>>> > >>>> This is an according patch that defines IN_RELOAD1_CC and uses it in > >>>> avr.h to define ELIMINABLE_REGS. > >>>> > >>>> This is only required for trunk. > >>>> > >>>> PR116326 occurred for some test case in avr-torture.exp, so I didn't > >>>> duplicate the test case. > >>>> > >>>> As it appears, this patch also fixes: > >>>> > >>>> https://gcc.gnu.org/PR116324 > >>>> https://gcc.gnu.org/PR116325 > >>>> https://gcc.gnu.org/PR116550 > >>>> > >>>> Johann > >>>> > >>>> -- > >>>> > >>>> AVR: target/116326 - Adjust ELIMINABLE_REGS to reload resp. LRA. > >>>> > >>>> PR target/116326 > >>>> gcc/ > >>>> * reload1.cc (IN_RELOAD1_CC): Define prior to all includes. > >>>> * config/avr/avr.h (ELIMINABLE_REGS): Depend on IN_RELOAD1_CC. > >>> I'm going to have to NAK this. It's too hackish, even with reload > going > >>> away.... > >>> > >>> Jeff > >> > >> So what is your proposal? > >> > >> I'd agree with Richard that we don't want to change reload > >> implementations such that they agree on ELIMINABLE_REGS. > >> > >> A different way would be to change reload1.cc such that it includes > >> reload.h prior to tm.h, i.e. prior to backend.h. This would make the > >> hack avr realm entirely, but it won't be trivially no-op. > >> > >> Johann > > > > Why not add RELOAD_ELIMINABLE_REGS and default it > > to ELIMINABLE_REGS? > > I can't find RELOAD_ELIMINABLE_REGS anywhere. > So are you proposing a new hook macro? > A new macro. > Johann > >
On Sat, Sep 7, 2024 at 8:22 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > On Sat, Sep 7, 2024, 11:07 AM Georg-Johann Lay <avr@gjlay.de> wrote: >> >> Am 07.09.24 um 19:51 schrieb H.J. Lu: >> > On Sat, Sep 7, 2024 at 10:49 AM Georg-Johann Lay <avr@gjlay.de> wrote: >> >> Am 07.09.24 um 18:35 schrieb Jeff Law: >> >>> On 9/6/24 3:40 AM, Georg-Johann Lay wrote: >> >>>> The reason for PR116326 is that LRA and reload require different >> >>>> ELIMINABLE_REGS for a multi-register frame pointer. As ELIMINABLE_REGS >> >>>> is used to initialize static const objects, it is not possible to make >> >>>> ELIMINABLE_REGS to depend on options or patch it in some target hook. >> >>>> >> >>>> It was also concluded that it is not desirable to adjust reload so that >> >>>> it behaves like LRA, but a hack like #define IN_RELOAD1_CC at the top >> >>>> of reload1.cc would be fine, see >> >>>> >> >>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116326#c8 >> >>>> >> >>>> This is an according patch that defines IN_RELOAD1_CC and uses it in >> >>>> avr.h to define ELIMINABLE_REGS. >> >>>> >> >>>> This is only required for trunk. >> >>>> >> >>>> PR116326 occurred for some test case in avr-torture.exp, so I didn't >> >>>> duplicate the test case. >> >>>> >> >>>> As it appears, this patch also fixes: >> >>>> >> >>>> https://gcc.gnu.org/PR116324 >> >>>> https://gcc.gnu.org/PR116325 >> >>>> https://gcc.gnu.org/PR116550 >> >>>> >> >>>> Johann >> >>>> >> >>>> -- >> >>>> >> >>>> AVR: target/116326 - Adjust ELIMINABLE_REGS to reload resp. LRA. >> >>>> >> >>>> PR target/116326 >> >>>> gcc/ >> >>>> * reload1.cc (IN_RELOAD1_CC): Define prior to all includes. >> >>>> * config/avr/avr.h (ELIMINABLE_REGS): Depend on IN_RELOAD1_CC. >> >>> I'm going to have to NAK this. It's too hackish, even with reload going >> >>> away.... >> >>> >> >>> Jeff >> >> >> >> So what is your proposal? >> >> >> >> I'd agree with Richard that we don't want to change reload >> >> implementations such that they agree on ELIMINABLE_REGS. >> >> >> >> A different way would be to change reload1.cc such that it includes >> >> reload.h prior to tm.h, i.e. prior to backend.h. This would make the >> >> hack avr realm entirely, but it won't be trivially no-op. >> >> >> >> Johann >> > >> > Why not add RELOAD_ELIMINABLE_REGS and default it >> > to ELIMINABLE_REGS? >> >> I can't find RELOAD_ELIMINABLE_REGS anywhere. >> So are you proposing a new hook macro? > > > A new macro. Sounds good to me btw. - the other alternative is to avoid having -m{,no-}lra and default to LRA now (not sure if that's feasible). Richard. >> >> Johann >>
AVR: target/116326 - Adjust ELIMINABLE_REGS to reload resp. LRA.
PR target/116326
gcc/
* reload1.cc (IN_RELOAD1_CC): Define prior to all includes.
* config/avr/avr.h (ELIMINABLE_REGS): Depend on IN_RELOAD1_CC.
diff --git a/gcc/config/avr/avr.h b/gcc/config/avr/avr.h
index 1cf4180e534..d540f2fcb13 100644
--- a/gcc/config/avr/avr.h
+++ b/gcc/config/avr/avr.h
@@ -308,11 +308,20 @@ enum reg_class {
#define STATIC_CHAIN_REGNUM ((AVR_TINY) ? 18 :2)
+#ifndef IN_RELOAD1_CC
+#define ELIMINABLE_REGS \
+ { \
+ { ARG_POINTER_REGNUM, STACK_POINTER_REGNUM }, \
+ { ARG_POINTER_REGNUM, FRAME_POINTER_REGNUM }, \
+ { FRAME_POINTER_REGNUM, STACK_POINTER_REGNUM } \
+ }
+#else
#define ELIMINABLE_REGS { \
{ ARG_POINTER_REGNUM, STACK_POINTER_REGNUM }, \
{ ARG_POINTER_REGNUM, FRAME_POINTER_REGNUM }, \
{ FRAME_POINTER_REGNUM, STACK_POINTER_REGNUM }, \
{ FRAME_POINTER_REGNUM + 1, STACK_POINTER_REGNUM + 1 } }
+#endif /* In reload1.cc ? */
#define INITIAL_ELIMINATION_OFFSET(FROM, TO, OFFSET) \
OFFSET = avr_initial_elimination_offset (FROM, TO)
diff --git a/gcc/reload1.cc b/gcc/reload1.cc
index 2e059b09970..cfa0be24f32 100644
--- a/gcc/reload1.cc
+++ b/gcc/reload1.cc
@@ -17,6 +17,15 @@ You should have received a copy of the GNU General Public License
along with GCC; see the file COPYING3. If not see
<http://www.gnu.org/licenses/>. */
+// PR116326: Reload and LRA use different representations of ELIMINABLE_REGS
+// for a multi-register frame-pointer like it is the case for avr. Since
+// ELIMINABLE_REGS is used to initialize a static const object, it is not
+// possible to cater for different ELIMINABLE_REGS by means of a command line
+// option like -m[no-]lra. But with the following macro, we can use #ifdef.
+// This hack was added to reload1.cc because after a complete Reload -> LRA
+// transition, this file will be removed.
+#define IN_RELOAD1_CC
+
#include "config.h"
#include "system.h"
#include "coretypes.h"