Re: rs6000: Generate an lxvp instead of two adjacent lxv instructions

[Segher Boessenkool <segher@kernel.crashing.org>]

On Thu, Jul 08, 2021 at 08:26:45PM -0500, Peter Bergner wrote:
> On 7/8/21 6:28 PM, Segher Boessenkool wrote:
> >>  	      int index = WORDS_BIG_ENDIAN ? i : nvecs - 1 - i;
> >> -	      rtx dst_i = gen_rtx_REG (reg_mode, reg + index);
> >> -	      emit_insn (gen_rtx_SET (dst_i, XVECEXP (src, 0, i)));
> >> +	      int index_next = WORDS_BIG_ENDIAN ? index + 1 : index - 1;
> > 
> > What does index_next mean?  The machine instructions do the same thing
> > in any endianness.
> 
> Yeah, I'm bad at coming up with names! :-)   So "index" is the index
> into XVECEXP (src, 0, ...) which is the operand that is to be assigned
> to regno.  "index_next" is the index into XVECEXP (src, 0, ...) which is
> the operand to be assigned to regno + 1 (ie, the next register of the
> even/odd register pair).  Whether the "next index" is index+1 or index-1
> is dependent on LE versus BE.

I would just call it "index1", or even "j" and "k" instead of "index"
and "index_next" :-)  "next" can put people on the wrong track (it did
me :-) )

> >> +	      /* If we are loading an even VSX register and our memory location
> >> +		 is adjacent to the next register's memory location (if any),
> >> +		 then we can load them both with one LXVP instruction.  */
> >> +	      if ((regno & 1) == 0
> >> +		  && VSX_REGNO_P (regno)
> >> +		  && MEM_P (XVECEXP (src, 0, index))
> >> +		  && MEM_P (XVECEXP (src, 0, index_next)))
> >> +		{
> >> +		  rtx base = WORDS_BIG_ENDIAN ? XVECEXP (src, 0, index)
> >> +					      : XVECEXP (src, 0, index_next);
> >> +		  rtx next = WORDS_BIG_ENDIAN ? XVECEXP (src, 0, index_next)
> >> +					      : XVECEXP (src, 0, index);
> > 
> > Please get rid of index_next, if you still have to do different code for
> > LE here -- it doesn't make the code any clearer (in fact I cannot follow
> > it at all anymore :-( )
> 
> We do need different code for LE versus BE.  So you want something like
> 
>   if (WORDS_BIG_ENDIAN) {...} else {...}
> 
> ...instead?  I can try that to see if the code is easier to read.

Yes exactly.  It will more directly say what it does, and there is no
"index_next" abstraction the reader has to absorb first.

> > So this converts pairs of lxv to an lxvp in only a very limited case,
> > right?  Can we instead do it more generically?  And what about stxvp?
> 
> Doing it more generically is my next TODO and that will cover both
> lxvp and stxvp.

Ah cool :-)

> My thought was to write a simple pass run at about
> the same time as our swap optimization pass to look for adjacent
> lxv's and stxv's and convert them into lxvp and stxvp.

So, very early, as soon as DF is set up.  Makes sense.

> However, that
> won't catch the above case, since the assemble/build pattern is not
> split until very late, so we still want the above change.

You probably should also have a peephole (whether you do it like here or
not :-) )

> Also, given the new pass will be more complicated than the above code,
> it will be a GCC 12 only change.

/nod

> Let me make the changes you want and I'll repost with what I come up with.

Thanks!  And thanks for the explanation.


Segher