[4/4] Fix autoinc cbranch
diff mbox series

Message ID 3b41f42c-188e-7afb-7554-e43971aa1c96@t-online.de
State New
Headers show
Series
  • Eliminate cc0 from m68k
Related show

Commit Message

Bernd Schmidt Nov. 13, 2019, 1:21 p.m. UTC
After the m68k cc0 conversion, there is one code quality regression that
I can see: we no longer generate autoinc addressing modes in
comparisons. This is because the parts of the compiler that generate
autoinc are unwilling to substitute into jumps.

If you look at the code in reload, you'll see that it's careful around
jumps at find_reload time, and the code to perform autoinc reloads does
try to put all the extra code before the instruction. LRA seems to have
copied most of that code.
Also, in the former cc0 reality, a compare wasn't really any different
from a jump on m68k: we can't have a reload after the instruction in
either case. Any kind of move or arithmetic would clobber the flags.

That leads me to believe that there is no issue with autoinc in jumps,
hence this patch. Bootstrapped and tested on the gcc135 machine
(powerpc64le-unknown-linux-gnu). I don't really expect this to get
approved; alternatively I could write some peepholes which would
generate the same code as long as register pressure doesn't get too high.


Bernd

Comments

Segher Boessenkool Nov. 19, 2019, 12:27 a.m. UTC | #1
On Wed, Nov 13, 2019 at 02:21:01PM +0100, Bernd Schmidt wrote:
> After the m68k cc0 conversion, there is one code quality regression that
> I can see: we no longer generate autoinc addressing modes in
> comparisons. This is because the parts of the compiler that generate
> autoinc are unwilling to substitute into jumps.
> 
> If you look at the code in reload, you'll see that it's careful around
> jumps at find_reload time, and the code to perform autoinc reloads does
> try to put all the extra code before the instruction. LRA seems to have
> copied most of that code.
> Also, in the former cc0 reality, a compare wasn't really any different
> from a jump on m68k: we can't have a reload after the instruction in
> either case. Any kind of move or arithmetic would clobber the flags.
> 
> That leads me to believe that there is no issue with autoinc in jumps,
> hence this patch. Bootstrapped and tested on the gcc135 machine
> (powerpc64le-unknown-linux-gnu). I don't really expect this to get
> approved; alternatively I could write some peepholes which would
> generate the same code as long as register pressure doesn't get too high.

I don't see why it wouldn't work?  If you apply it now we will still
have a lot of time to find problems with it, if those exist.

The combine parts are okay for trunk, if you keep an eye out :-)


Segher
Bernd Schmidt Nov. 24, 2019, 2:19 p.m. UTC | #2
On 11/19/19 1:27 AM, Segher Boessenkool wrote:
> The combine parts are okay for trunk, if you keep an eye out :-)

Thanks, now committed. That leaves the auto-inc-dec part. Since we're
being adventurous, I've also bootstrapped and tested the following in
the meantime (on the gcc135 machine). This just deletes the tests for
jumps entirely rather than hedging bets.


Bernd
Segher Boessenkool Nov. 24, 2019, 5:39 p.m. UTC | #3
Hi!

On Sun, Nov 24, 2019 at 03:19:49PM +0100, Bernd Schmidt wrote:
> -      /* This continue is deliberate.  We do not want the uses of the
> -	 jump put into reg_next_use because it is not considered safe to
> -	 combine a preincrement with a jump.  */
> -      if (JUMP_P (insn))
> -	continue;

Huh, I wonder where that came from.  More archaelogy :-)

(This patch looks fine to me, fwiw).


Segher
Segher Boessenkool Nov. 24, 2019, 7:43 p.m. UTC | #4
On Sun, Nov 24, 2019 at 11:39:05AM -0600, Segher Boessenkool wrote:
> On Sun, Nov 24, 2019 at 03:19:49PM +0100, Bernd Schmidt wrote:
> > -      /* This continue is deliberate.  We do not want the uses of the
> > -	 jump put into reg_next_use because it is not considered safe to
> > -	 combine a preincrement with a jump.  */
> > -      if (JUMP_P (insn))
> > -	continue;
> 
> Huh, I wonder where that came from.  More archaelogy :-)

It came in as r115135 on the dataflow-branch.  Patch is at
https://gcc.gnu.org/ml/gcc-patches/2006-07/msg00037.html .

And that is where I lost track.

But.  Allowing autoinc into jump insns means those jump insns may then
eventually need an output reload; it may just have been because of that?


Segher
Bernd Schmidt Nov. 24, 2019, 7:55 p.m. UTC | #5
On 11/24/19 8:43 PM, Segher Boessenkool wrote:
> But.  Allowing autoinc into jump insns means those jump insns may then
> eventually need an output reload; it may just have been because of that?

That's almost certainly the reasoning, but as I pointed out in my
original mail - reload is careful around autoincs and emits the reload
sequence before the reloaded insn.

For example, see inc_for_reload:
      /* Postincrement.
	 Because this might be a jump insn or a compare, and because RELOADREG
	 may not be available after the insn in an input reload, we must do
	 the incrementation before the insn being reloaded for.

On m68k with cc0 this is already necessary, because even a cmp insn
cannot have output reloads (they would overwrite the flags). This is why
I think it should be safe to allow them in jumps too.


Bernd
Segher Boessenkool Nov. 24, 2019, 9:42 p.m. UTC | #6
On Sun, Nov 24, 2019 at 08:55:37PM +0100, Bernd Schmidt wrote:
> On 11/24/19 8:43 PM, Segher Boessenkool wrote:
> > But.  Allowing autoinc into jump insns means those jump insns may then
> > eventually need an output reload; it may just have been because of that?
> 
> That's almost certainly the reasoning, but as I pointed out in my
> original mail - reload is careful around autoincs and emits the reload
> sequence before the reloaded insn.

Yes, and that isn't anything new either.

Does LRA do this as well?  ...  Yes it does, see lra-constraints.c, search
for "Because this might be a jump" as well.

> For example, see inc_for_reload:
>       /* Postincrement.
> 	 Because this might be a jump insn or a compare, and because RELOADREG
> 	 may not be available after the insn in an input reload, we must do
> 	 the incrementation before the insn being reloaded for.
> 
> On m68k with cc0 this is already necessary, because even a cmp insn
> cannot have output reloads (they would overwrite the flags). This is why
> I think it should be safe to allow them in jumps too.

Yeah, auto-modify makes my head hurt :-)

Output reloads definitely are *not* safe in jumps, but auto-modify seems
to be indeed.


Segher

Patch
diff mbox series

	* auto-inc-dec.c (merge_in_block): Allow jumps.
	* combine.c (can_combine_p): Allow jumps in autoinc.

diff --git a/gcc/auto-inc-dec.c b/gcc/auto-inc-dec.c
index bdb6efa..6dab135 100644
--- a/gcc/auto-inc-dec.c
+++ b/gcc/auto-inc-dec.c
@@ -1441,10 +1441,11 @@  merge_in_block (int max_reg, basic_block bb)
          continue;
        }
 
-      /* This continue is deliberate.  We do not want the uses of the
-        jump put into reg_next_use because it is not considered safe to
-        combine a preincrement with a jump.  */
-      if (JUMP_P (insn))
+      /* We used to skip jump insns, but both reload and LRA seem to
+        take precautions not to perform autoinc reloads after a jump or
+        a comparison.  Allow them for regular autoinc only (for test
+        coverage reasons more than anything).  */
+      if ((HAVE_PRE_MODIFY_REG || HAVE_POST_MODIFY_REG) && JUMP_P (insn))
        continue;
 
       if (dump_file)
diff --git a/gcc/combine.c b/gcc/combine.c
index 857ea30..e9e1464 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2119,15 +2118,12 @@  can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED,
 
   /* If INSN contains an autoincrement or autodecrement, make sure that
      register is not used between there and I3, and not already used in
-     I3 either.  Neither must it be used in PRED or SUCC, if they exist.
-     Also insist that I3 not be a jump; if it were one
-     and the incremented register were spilled, we would lose.  */
+     I3 either.  Neither must it be used in PRED or SUCC, if they exist.  */
 
   if (AUTO_INC_DEC)
     for (link = REG_NOTES (insn); link; link = XEXP (link, 1))
       if (REG_NOTE_KIND (link) == REG_INC
-         && (JUMP_P (i3)
-             || reg_used_between_p (XEXP (link, 0), insn, i3)
+         && (reg_used_between_p (XEXP (link, 0), insn, i3)
              || (pred != NULL_RTX
                  && reg_overlap_mentioned_p (XEXP (link, 0), PATTERN (pred)))
              || (pred2 != NULL_RTX