[4/4] Fix autoinc cbranch

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

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

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

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

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

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

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