Run the combine part of combine_and_move_insns even if -fsched-pressure

Run the combine part of combine_and_move_insns even if -fsched-pressure

[Richard Sandiford]

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.

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} } } */

Re: Run the combine part of combine_and_move_insns even if -fsched-pressure

[Segher Boessenkool]

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

Re: Run the combine part of combine_and_move_insns even if -fsched-pressure

[Richard Sandiford]

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

Re: Run the combine part of combine_and_move_insns even if -fsched-pressure

[Segher Boessenkool]

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

Re: Run the combine part of combine_and_move_insns even if -fsched-pressure

[Segher Boessenkool]

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

Re: Run the combine part of combine_and_move_insns even if -fsched-pressure

[Jeff Law]

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