| Message ID | mptimr86duk.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Run the combine part of combine_and_move_insns even if -fsched-pressure | expand |
Hi Richard, On Wed, Aug 07, 2019 at 07:16:03PM +0100, Richard Sandiford wrote: > The main IRA routine includes the code: > > /* Don't move insns if live range shrinkage or register > pressure-sensitive scheduling were done because it will not > improve allocation but likely worsen insn scheduling. */ > if (optimize > && !flag_live_range_shrinkage > && !(flag_sched_pressure && flag_schedule_insns)) > combine_and_move_insns (); > > The comment about not moving insns for pressure-sensitive scheduling > makes sense, but I think the combine part of combine_and_move_insns is > still useful, since it's folding a set of an equivalent value into its > single user and so eliminates the need for one register altogether. During which pass are the newly combined instructions created? If not late, why does combine refuse to do this combination? Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > Hi Richard, > > On Wed, Aug 07, 2019 at 07:16:03PM +0100, Richard Sandiford wrote: >> The main IRA routine includes the code: >> >> /* Don't move insns if live range shrinkage or register >> pressure-sensitive scheduling were done because it will not >> improve allocation but likely worsen insn scheduling. */ >> if (optimize >> && !flag_live_range_shrinkage >> && !(flag_sched_pressure && flag_schedule_insns)) >> combine_and_move_insns (); >> >> The comment about not moving insns for pressure-sensitive scheduling >> makes sense, but I think the combine part of combine_and_move_insns is >> still useful, since it's folding a set of an equivalent value into its >> single user and so eliminates the need for one register altogether. > > During which pass are the newly combined instructions created? If not > late, why does combine refuse to do this combination? During if_after_combine. I should have said, but these tests aren't actually the motivating examples. They're just something I found while trawling the testsuite for a nice short reproducer (gcc.c-torture/compile/scc.c). The justification really stands independently of whichever tests end up being bundled with the patch. While we have code to do combinations at this point in the pipeline, I think we should use it independently of -fsched-pressure. I'm pretty sure the current -fsched-pressure condition was added specifically to prevent moves (which makes a lot of sense) rather than to stop combinations too. Thanks, Richard
On Wed, Aug 07, 2019 at 11:21:48PM +0100, Richard Sandiford wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Wed, Aug 07, 2019 at 07:16:03PM +0100, Richard Sandiford wrote: > >> The main IRA routine includes the code: > >> > >> /* Don't move insns if live range shrinkage or register > >> pressure-sensitive scheduling were done because it will not > >> improve allocation but likely worsen insn scheduling. */ > >> if (optimize > >> && !flag_live_range_shrinkage > >> && !(flag_sched_pressure && flag_schedule_insns)) > >> combine_and_move_insns (); > >> > >> The comment about not moving insns for pressure-sensitive scheduling > >> makes sense, but I think the combine part of combine_and_move_insns is > >> still useful, since it's folding a set of an equivalent value into its > >> single user and so eliminates the need for one register altogether. > > > > During which pass are the newly combined instructions created? If not > > late, why does combine refuse to do this combination? > > During if_after_combine. So immediately after combine. Why did combine refuse to do this itself? What am I missing? Segher
On Wed, Aug 07, 2019 at 05:26:45PM -0500, Segher Boessenkool wrote: > On Wed, Aug 07, 2019 at 11:21:48PM +0100, Richard Sandiford wrote: > > Segher Boessenkool <segher@kernel.crashing.org> writes: > > > On Wed, Aug 07, 2019 at 07:16:03PM +0100, Richard Sandiford wrote: > > >> The main IRA routine includes the code: > > >> > > >> /* Don't move insns if live range shrinkage or register > > >> pressure-sensitive scheduling were done because it will not > > >> improve allocation but likely worsen insn scheduling. */ > > >> if (optimize > > >> && !flag_live_range_shrinkage > > >> && !(flag_sched_pressure && flag_schedule_insns)) > > >> combine_and_move_insns (); > > >> > > >> The comment about not moving insns for pressure-sensitive scheduling > > >> makes sense, but I think the combine part of combine_and_move_insns is > > >> still useful, since it's folding a set of an equivalent value into its > > >> single user and so eliminates the need for one register altogether. > > > > > > During which pass are the newly combined instructions created? If not > > > late, why does combine refuse to do this combination? > > > > During if_after_combine. > > So immediately after combine. > > Why did combine refuse to do this itself? What am I missing? Oh, duh, these are *new* instructions created by that ce2 pass. Duh :-) Segher
On 8/7/19 12:16 PM, Richard Sandiford wrote: > The main IRA routine includes the code: > > /* Don't move insns if live range shrinkage or register > pressure-sensitive scheduling were done because it will not > improve allocation but likely worsen insn scheduling. */ > if (optimize > && !flag_live_range_shrinkage > && !(flag_sched_pressure && flag_schedule_insns)) > combine_and_move_insns (); > > The comment about not moving insns for pressure-sensitive scheduling > makes sense, but I think the combine part of combine_and_move_insns is > still useful, since it's folding a set of an equivalent value into its > single user and so eliminates the need for one register altogether. > > (That also means that it's likely to undo live range shrinkage in > some cases, so I think the blanket skip still makes sense there.) > > Tested on aarch64-linux-gnu, aarch64_be-elf and x86_64-linux-gnu. > OK to install? > > Richard > > > 2019-08-07 Richard Sandiford <richard.sandiford@arm.com> > > gcc/ > * ira.c (combine_and_move_insns): Don't move insns if > pressure-sensitive scheduling is enabled. > (ira): Remove check for pressure-sensitive scheduling here. > > gcc/testsuite/ > * gcc.target/aarch64/csinc-3.c: New test. > * gcc.target/aarch64/csinv-2.c: Likewise. OK jeff
Index: gcc/ira.c =================================================================== --- gcc/ira.c 2019-07-10 19:41:27.159891908 +0100 +++ gcc/ira.c 2019-08-07 19:12:57.945375459 +0100 @@ -3748,6 +3748,11 @@ combine_and_move_insns (void) auto_bitmap cleared_regs; int max = max_reg_num (); + /* Don't move insns if register pressure-sensitive scheduling was + done because it will not improve allocation but likely worsen insn + scheduling. */ + bool allow_move_p = !(flag_sched_pressure && flag_schedule_insns); + for (int regno = FIRST_PSEUDO_REGISTER; regno < max; regno++) { if (!reg_equiv[regno].replace) @@ -3829,7 +3834,7 @@ combine_and_move_insns (void) /* Move the initialization of the register to just before USE_INSN. Update the flow information. */ - else if (prev_nondebug_insn (use_insn) != def_insn) + else if (allow_move_p && prev_nondebug_insn (use_insn) != def_insn) { rtx_insn *new_insn; @@ -5307,12 +5312,8 @@ ira (FILE *f) reg_equiv = XCNEWVEC (struct equivalence, max_reg_num ()); update_equiv_regs (); - /* Don't move insns if live range shrinkage or register - pressure-sensitive scheduling were done because it will not - improve allocation but likely worsen insn scheduling. */ - if (optimize - && !flag_live_range_shrinkage - && !(flag_sched_pressure && flag_schedule_insns)) + /* This subpass could undo the effects of live range shrinkage. */ + if (optimize && !flag_live_range_shrinkage) combine_and_move_insns (); /* Gather additional equivalences with memory. */ Index: gcc/testsuite/gcc.target/aarch64/csinc-3.c =================================================================== --- /dev/null 2019-07-30 08:53:31.317691683 +0100 +++ gcc/testsuite/gcc.target/aarch64/csinc-3.c 2019-08-07 19:12:57.945375459 +0100 @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +int +foo (int a, int b) +{ + if (a < 0) + return 1; + if (a == 0) + return b; + return b + 1; +} + +/* { dg-final { scan-assembler-not {\tmov\tw[0-9]+, 1\n} } } */ +/* { dg-final { scan-assembler {\tcsinc\tw[0-9]+, w[0-9]+, wzr, ge\n} } } */ Index: gcc/testsuite/gcc.target/aarch64/csinv-2.c =================================================================== --- /dev/null 2019-07-30 08:53:31.317691683 +0100 +++ gcc/testsuite/gcc.target/aarch64/csinv-2.c 2019-08-07 19:12:57.945375459 +0100 @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +int +foo (int a, int b) +{ + if (a < 0) + return -1; + if (a == 0) + return 0; + return 1; +} + +/* { dg-final { scan-assembler-not {\tmov\tw[0-9]+, -1\n} } } */ +/* { dg-final { scan-assembler {\tcsinv\tw[0-9]+, w[0-9]+, wzr, ge\n} } } */
The main IRA routine includes the code: /* Don't move insns if live range shrinkage or register pressure-sensitive scheduling were done because it will not improve allocation but likely worsen insn scheduling. */ if (optimize && !flag_live_range_shrinkage && !(flag_sched_pressure && flag_schedule_insns)) combine_and_move_insns (); The comment about not moving insns for pressure-sensitive scheduling makes sense, but I think the combine part of combine_and_move_insns is still useful, since it's folding a set of an equivalent value into its single user and so eliminates the need for one register altogether. (That also means that it's likely to undo live range shrinkage in some cases, so I think the blanket skip still makes sense there.) Tested on aarch64-linux-gnu, aarch64_be-elf and x86_64-linux-gnu. OK to install? Richard 2019-08-07 Richard Sandiford <richard.sandiford@arm.com> gcc/ * ira.c (combine_and_move_insns): Don't move insns if pressure-sensitive scheduling is enabled. (ira): Remove check for pressure-sensitive scheduling here. gcc/testsuite/ * gcc.target/aarch64/csinc-3.c: New test. * gcc.target/aarch64/csinv-2.c: Likewise.