Message ID | mpttuo6wpux.fsf@arm.com |
---|---|
State | New |
Headers | show |
Series | rtlanal: Don't assume that calls write to a global SP [PR99596] | expand |
On Fri, Apr 16, 2021 at 10:17:10AM +0100, Richard Sandiford via Gcc-patches wrote: > gcc/ > PR rtl-optimization/99596 > * rtlanal.c (rtx_properties::try_to_add_insn): Don't add global > register accesses for const calls. Assume that pure functions > can only read from global registers. Ignore cases in which > the stack pointer has been marked global. > > gcc/testsuite/ > PR rtl-optimization/99596 > * gcc.target/arm/pr99596.c: New test. LGTM, thanks. Jakub
On Fri, Apr 16, 2021 at 12:01 PM Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > This patch is a GCC 11 regression caused by the rtl-ssa code. > Normally we treat calls as containing a potential set of a global > register, but DF makes a sensible exception for the stack pointer: > > if (i == STACK_POINTER_REGNUM) > /* The stack ptr is used (honorarily) by a CALL insn. */ > df_ref_record (DF_REF_BASE, collection_rec, regno_reg_rtx[i], > NULL, bb, insn_info, DF_REF_REG_USE, > DF_REF_CALL_STACK_USAGE | flags); > else if (global_regs[i]) > { > /* Calls to const functions cannot access any global registers and > calls to pure functions cannot set them. All other calls may > reference any of the global registers, so they are recorded as > used. */ > > The only DF definition of SP was therefore the one in the entry block. > However, the rtlanal.c rtx_properties code (wrongly) assumed that calls > also clobbered the global SP. This led to multiple definitions of SP > when we only expected one. > > This patch tightens the rtlanal.c handling of global registers > to match the DF approach. > > Tested on aarch64-linux-gnu, arm-linux-gnueabihf, armeb-eabi and > x86_64-linux-gnu. OK to install? Makes sense for the stack pointer handling and also for the const/pure call adjustment. I assume the latter wasn't necessary to fix the testcase though? Still OK. Thanks, Richard. > Richard > > > gcc/ > PR rtl-optimization/99596 > * rtlanal.c (rtx_properties::try_to_add_insn): Don't add global > register accesses for const calls. Assume that pure functions > can only read from global registers. Ignore cases in which > the stack pointer has been marked global. > > gcc/testsuite/ > PR rtl-optimization/99596 > * gcc.target/arm/pr99596.c: New test. > --- > gcc/rtlanal.c | 20 +++++++++++++++----- > gcc/testsuite/gcc.target/arm/pr99596.c | 18 ++++++++++++++++++ > 2 files changed, 33 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/arm/pr99596.c > > diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c > index c35386bccbd..170420a610b 100644 > --- a/gcc/rtlanal.c > +++ b/gcc/rtlanal.c > @@ -2311,15 +2311,25 @@ rtx_properties::try_to_add_insn (const rtx_insn *insn, bool include_notes) > { > if (CALL_P (insn)) > { > - /* Adding the global registers first removes a situation in which > + /* Non-const functions can read from global registers. Impure > + functions can also set them. > + > + Adding the global registers first removes a situation in which > a fixed-form clobber of register R could come before a real set > of register R. */ > - if (!hard_reg_set_empty_p (global_reg_set)) > + if (!hard_reg_set_empty_p (global_reg_set) > + && !RTL_CONST_CALL_P (insn)) > { > - unsigned int flags = (rtx_obj_flags::IS_READ > - | rtx_obj_flags::IS_WRITE); > + unsigned int flags = rtx_obj_flags::IS_READ; > + if (!RTL_PURE_CALL_P (insn)) > + flags |= rtx_obj_flags::IS_WRITE; > for (unsigned int regno = 0; regno < FIRST_PSEUDO_REGISTER; ++regno) > - if (global_regs[regno] && ref_iter != ref_end) > + /* As a special case, the stack pointer is invariant across calls > + even if it has been marked global; see the corresponding > + handling in df_get_call_refs. */ > + if (regno != STACK_POINTER_REGNUM > + && global_regs[regno] > + && ref_iter != ref_end) > *ref_iter++ = rtx_obj_reference (regno, flags, > reg_raw_mode[regno], 0); > } > diff --git a/gcc/testsuite/gcc.target/arm/pr99596.c b/gcc/testsuite/gcc.target/arm/pr99596.c > new file mode 100644 > index 00000000000..2b8b4c87e96 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/arm/pr99596.c > @@ -0,0 +1,18 @@ > +/* { dg-options "-Os -mtune=xscale" } */ > + > +register int a asm("sp"); > +extern int b; > +typedef struct { > + long c[16 * 8 / 32]; > +} d; > +int e; > +int f; > +int g; > +d h; > +int j(int, int, int, d); > +int i(void) { > + for (;;) { > + b &&j(e, f, g, h); > + j(e, f, g, h); > + } > +}
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c index c35386bccbd..170420a610b 100644 --- a/gcc/rtlanal.c +++ b/gcc/rtlanal.c @@ -2311,15 +2311,25 @@ rtx_properties::try_to_add_insn (const rtx_insn *insn, bool include_notes) { if (CALL_P (insn)) { - /* Adding the global registers first removes a situation in which + /* Non-const functions can read from global registers. Impure + functions can also set them. + + Adding the global registers first removes a situation in which a fixed-form clobber of register R could come before a real set of register R. */ - if (!hard_reg_set_empty_p (global_reg_set)) + if (!hard_reg_set_empty_p (global_reg_set) + && !RTL_CONST_CALL_P (insn)) { - unsigned int flags = (rtx_obj_flags::IS_READ - | rtx_obj_flags::IS_WRITE); + unsigned int flags = rtx_obj_flags::IS_READ; + if (!RTL_PURE_CALL_P (insn)) + flags |= rtx_obj_flags::IS_WRITE; for (unsigned int regno = 0; regno < FIRST_PSEUDO_REGISTER; ++regno) - if (global_regs[regno] && ref_iter != ref_end) + /* As a special case, the stack pointer is invariant across calls + even if it has been marked global; see the corresponding + handling in df_get_call_refs. */ + if (regno != STACK_POINTER_REGNUM + && global_regs[regno] + && ref_iter != ref_end) *ref_iter++ = rtx_obj_reference (regno, flags, reg_raw_mode[regno], 0); } diff --git a/gcc/testsuite/gcc.target/arm/pr99596.c b/gcc/testsuite/gcc.target/arm/pr99596.c new file mode 100644 index 00000000000..2b8b4c87e96 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr99596.c @@ -0,0 +1,18 @@ +/* { dg-options "-Os -mtune=xscale" } */ + +register int a asm("sp"); +extern int b; +typedef struct { + long c[16 * 8 / 32]; +} d; +int e; +int f; +int g; +d h; +int j(int, int, int, d); +int i(void) { + for (;;) { + b &&j(e, f, g, h); + j(e, f, g, h); + } +}