rs6000: Make the ctr* patterns allow ints in vector regs (PR71763)

On Thu, Jul 07, 2016 at 02:58:17AM +0000, Segher Boessenkool wrote:
> Similar to PR70098, which is about integers in floating point registers,
> we can have the completely analogous problem with vector registers as well
> now that we allow integers in vector registers.  So, this patch solves it
> in the same way.  This only works for targets with direct move.
> 
> To recap: register allocation can decide to put an integer mode value in
> a floating point or vector register.  If that register is used in a bd*z
> instruction, which is a jump instruction, reload can not do an output
> reload on it (it does not do output reloads on any jump insns), so the
> float or vector register will remain, and we have to allow it here or
> recog will ICE.  Later on we will split this to valid instructions,
> including a move from that fp/vec register to an int register; it is this
> move that will still fail (PR70098) if we do not have direct move enabled.

BTW, both pr70098 and pr71763 are triggered by combine, not
loop-doloop as I was thinking earlier.  See rtl dumps for the
testcases.  I doubt the "optimization" done by combine here is worth
keeping, since loop-doloop.c ought to already handle the benficial
inner loop use of ctr.  Elsewhere we typically end up with an insn
that needs splitting back to the original sequence.  So we could avoid
creating trouble for ourselves with the following patch.

Bootstrap and regression test powerpc64le-linux and powerpc64-linux in
progress.

	* config/rs6000/rs6000.md (UNSPEC_DONT_COMBINE): New unspec.
	(ctr<mode>): Add unspec.
	(ctr<mode>_internal* and splitters): Likewise.  Renumber.

On Fri, Jul 08, 2016 at 01:28:05AM +0930, Alan Modra wrote:
> BTW, both pr70098 and pr71763 are triggered by combine, not
> loop-doloop as I was thinking earlier.  See rtl dumps for the
> testcases.  I doubt the "optimization" done by combine here is worth
> keeping, since loop-doloop.c ought to already handle the benficial
> inner loop use of ctr.  Elsewhere we typically end up with an insn
> that needs splitting back to the original sequence.  So we could avoid
> creating trouble for ourselves with the following patch.
> 
> Bootstrap and regression test powerpc64le-linux and powerpc64-linux in
> progress.
> 
> 	* config/rs6000/rs6000.md (UNSPEC_DONT_COMBINE): New unspec.
> 	(ctr<mode>): Add unspec.
> 	(ctr<mode>_internal* and splitters): Likewise.  Renumber.

The regression tests passed.  I've been looking at differences in
gcc/*.o and find many cases like the following.

orig/combine.o
    1508:	01 00 3f 2c 	cmpdi   r31,1
    150c:	ff ff ff 3b 	addi    r31,r31,-1
    1510:	dc fe 82 41 	beq     13ec
patched/combine.o
    1508:	ff ff ff 37 	addic.  r31,r31,-1
    150c:	e0 fe 82 41 	beq     13ec

Combine transforms the first sequence to the second, then further
transforms that to a bdz (ctr<mode>).  When that fails to get ctr
allocated, the splitter takes us all the way back to the three insn
sequence..

There's also a quite interesting case involving this nested loop in
real.c:real_to_hexadecimal.

  for (i = SIGSZ - 1; i >= 0; --i)
    for (j = HOST_BITS_PER_LONG - 4; j >= 0; j -= 4)
      {
	*p++ = "0123456789abcdef"[(r->sig[i] >> j) & 15];
	if (--digits == 0)
	  goto out;
      }

With the patch we use ctr for the inner loop.  With unpatched gcc
combine generates ctr<mode> for the outer loop, which of course uses
ctr and isn't profitable with an inner loop using ctr.  Vagaries of
the register allocator result in the outer loop using ctr with the
inner one losing.  Oops, we generally want inner loops to be more
highly optimized.

On Fri, Jul 08, 2016 at 12:37:55PM +0930, Alan Modra wrote:
> The regression tests passed.  I've been looking at differences in
> gcc/*.o and find many cases like the following.
> 
> orig/combine.o
>     1508:	01 00 3f 2c 	cmpdi   r31,1
>     150c:	ff ff ff 3b 	addi    r31,r31,-1
>     1510:	dc fe 82 41 	beq     13ec
> patched/combine.o
>     1508:	ff ff ff 37 	addic.  r31,r31,-1
>     150c:	e0 fe 82 41 	beq     13ec
> 
> Combine transforms the first sequence to the second, then further
> transforms that to a bdz (ctr<mode>).  When that fails to get ctr
> allocated, the splitter takes us all the way back to the three insn
> sequence..

It used to do the addic. insn.  When I made the carry bit exposed to GCC,
it no longer was possible to always split to addic. though (CA might be
live there already).  Since the splitter should seldomly be used at all,
it now never splits to addic. (and addic. also is slower on some machines,
it is cracked, longer latency than you get with the compare to 1).

> With the patch we use ctr for the inner loop.  With unpatched gcc
> combine generates ctr<mode> for the outer loop, which of course uses
> ctr and isn't profitable with an inner loop using ctr.  Vagaries of
> the register allocator result in the outer loop using ctr with the
> inner one losing.  Oops, we generally want inner loops to be more
> highly optimized.

Lovely :-)


Segher

On Fri, Jul 08, 2016 at 01:28:05AM +0930, Alan Modra wrote:
> BTW, both pr70098 and pr71763 are triggered by combine, not
> loop-doloop as I was thinking earlier.  See rtl dumps for the
> testcases.  I doubt the "optimization" done by combine here is worth
> keeping, since loop-doloop.c ought to already handle the benficial
> inner loop use of ctr.  Elsewhere we typically end up with an insn
> that needs splitting back to the original sequence.  So we could avoid
> creating trouble for ourselves with the following patch.

I agree on the approach; if there are any missed optimisation because
of it, it doesn't weigh up to the frequentish pessimisation we have now.

One case it will prevent it bdz before a bdnz loop (for a loop count of
zero), but we usually do not generate that anyway, and it isn't obvious
it is faster anyway (or smaller, even).

> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 7d9c660..b2d1118 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -148,6 +148,7 @@
>     UNSPEC_IEEE128_MOVE
>     UNSPEC_IEEE128_CONVERT
>     UNSPEC_SIGNBIT
> +   UNSPEC_DONT_COMBINE
>    ])

That is a pretty horrible name.  Combine can combine such insns just fine;
it won't make up the unspec out of thin air though.

It seems you want to use this in other cases that should not be invented
by combine as well, but that won't work: combine could then morph one of
those into another kind.

Maybe just UNSPEC_BDZ?  UNSPEC_DOLOOP?

> -(define_insn "*ctr<mode>_internal5"
> +(define_insn "*ctr<mode>_internal3"

Please don't rename the patterns, not if you don't make better names.


Thanks, this should be a nice improvement,


Segher

On Fri, Jul 08, 2016 at 04:17:36AM -0500, Segher Boessenkool wrote:
> Maybe just UNSPEC_BDZ?  UNSPEC_DOLOOP?

Committed revision 238207, using UNSPEC_DOLOOP.

Should we backport this?  At least Alan's UNSPEC_DOLOOP part?

- David


On Mon, Jul 11, 2016 at 6:30 AM, Alan Modra <amodra@gmail.com> wrote:
> On Fri, Jul 08, 2016 at 04:17:36AM -0500, Segher Boessenkool wrote:
>> Maybe just UNSPEC_BDZ?  UNSPEC_DOLOOP?
>
> Committed revision 238207, using UNSPEC_DOLOOP.
>
> --
> Alan Modra
> Australia Development Lab, IBM

On Mon, Jul 11, 2016 at 06:54:44AM -0400, David Edelsohn wrote:
> Should we backport this?  At least Alan's UNSPEC_DOLOOP part?

The *wi is a bugfix; I'll backport it, just like the *d (already did
that one, 6 and 5; do we want 4.9 as well?)

Alan's last patch would be good to have as well, it is arguably a bugfix,
and it avoids most of the problematic cases.


Segher

On Mon, Jul 11, 2016 at 06:54:44AM -0400, David Edelsohn wrote:
> Should we backport this?  At least Alan's UNSPEC_DOLOOP part?

Alan backported this to 6 (I unfortunately removed gcc-patches
from cc:).


Segher