, PR target/93937, Fix variable vec_extract insn that will never match

As part of my work in adding support for -mcpu=future, I noticed an insn that
would never match.

Here is the insn:

(define_insn_and_split "*vsx_extract_<mode>_<VS_scalar>mode_var"
  [(set (match_operand:<VS_scalar> 0 "gpc_reg_operand" "=r,r,r")
	(zero_extend:<VS_scalar>
	 (unspec:<VSX_EXTRACT_I:VS_scalar>
	  [(match_operand:VSX_EXTRACT_I 1 "input_operand" "v,v,Q")
	   (match_operand:DI 2 "gpc_reg_operand" "r,r,r")]
	  UNSPEC_VSX_EXTRACT)))
   (clobber (match_scratch:DI 3 "=r,r,&b"))
   (clobber (match_scratch:V2DI 4 "=X,&v,X"))]
  "VECTOR_MEM_VSX_P (<VSX_EXTRACT_I:MODE>mode) && TARGET_DIRECT_MOVE_64BIT"
  "#"
  "&& reload_completed"
  [(const_int 0)]
{
  machine_mode smode = <VS_scalar>mode;
  rs6000_split_vec_extract_var (gen_rtx_REG (smode, REGNO (operands[0])),
				operands[1], operands[2],
				operands[3], operands[4]);
  DONE;
}
  [(set_attr "isa" "p9v,*,*")])

It will never match, because the zero_extend result is the same mode as the
input, so the machine independent parts of the compiler would never insert the
zero extend.  Instead, an explicit extend is generated to convert the type to
DImode.  In addition, there is the need to split the insn into two parts, one
that handles the register and the other the memory optimization such as was
done for the other 3 variable insns as part of PR target/93932.

There is a wider issue to optimize all cases of vec_extract to do the sign,
zero, and float extension automatically when we are loading from memory, which
is PR target/93230.  I have patches for all of the cases for 93230, but they
will need to wait until GCC 11 opens up.

But it irks me to have this pattern that mostly was correct, but it would never
match.  As I see it, there are 4 options:

1) Delete the insn completely, since it doesn't match, and then put in code
later to cover the case when PR target/93230 is done.

2) Ignore the patterns in the source code, accepting that they are useless, and
will be fixed some day.

3) Patch the existing insns so that they do match, but don't add all of the
other options that could be added (adding sign extension, adding the ability to
load the value into vector registers with ISA 2.07, optimizing vec_extract
being cnverted to floating point to avoid direct moves, etc.).

4) Do all of the possibilities now.  The trouble is due to the number of
special cases, this can add up to a number of new insns (for example, dealing
with HImode/QImode also being zero extended to SImode in addition to DImode,
dealing with QImode not having a sign extending load, dealing with SImode that
can load into the vector registers at ISA 2.05/2.06 which HI/QI modes need
2.07, etc.).

Given we are in stage 4, I think #4 is not appropriate (but if you want, I can
do the patches).

I would prefer to either delete the patterns (#1) or fix them in a limited
extent (#3).  I would prefer not to leave them unchanged in vsx.md, but
obviously that is the simplest approach.

This patch implements #3, fixing the insns as written, but not extending them
to handle all of the other special cases.

I have built compilers on both big endian and little endian PowerPC Linux
systems.  These patches fix the instruction counts of the three tests that now
eliminate the zero_extend operation.  Can I check these patches into the master
branch?

[gcc]
2020-02-28  Michael Meissner  <meissner@linux.ibm.com>

	PR target/93937
	* config/rs6000/vsx.md (vsx_extract_<mode>_<VS_scalar>mode_var):
	Delete, replace with vsx_extract_<mode>_uns_di_var.
	(vsx_extract_<mode>_uns_di_var): Replacement insn that will match
	zero extensions properly.  Restrict the vector to be in a
	register.
	(vsx_extract_<mode>_uns_di_var_load): New insn to handle variable
	vector extract from memory and combine it with a zero extend to
	DImode.

[gcc/testsuite]
2020-02-28  Michael Meissner  <meissner@linux.ibm.com>

	PR target/93937
	* gcc.target/powerpc/fold-vec-extract-char.p8.c: Update
	instruction counts.
	* gcc.target/powerpc/fold-vec-extract-int.p8.c: Update
	instruction counts.
	* gcc.target/powerpc/fold-vec-extract-short.p8.c: Update
	instruction counts.

Hi!

On Fri, Feb 28, 2020 at 12:32:06AM -0500, Michael Meissner wrote:
> As part of my work in adding support for -mcpu=future, I noticed an insn that
> would never match.

> It will never match, because the zero_extend result is the same mode as the
> input, so the machine independent parts of the compiler would never insert the
> zero extend.

It's not valid RTL, even:
  @findex zero_extend
  @item (zero_extend:@var{m} @var{x})
  Represents the result of zero-extending the value @var{x}
  to machine mode @var{m}.  @var{m} must be a fixed-point mode
  and @var{x} a fixed-point value of a mode narrower than @var{m}.

> There is a wider issue to optimize all cases of vec_extract to do the sign,
> zero, and float extension automatically when we are loading from memory, which
> is PR target/93230.  I have patches for all of the cases for 93230, but they
> will need to wait until GCC 11 opens up.

If you don't use reload_completed in the split condition you do not have
this problem (in the normal case).  Please work on that?

> But it irks me to have this pattern that mostly was correct, but it would never
> match.  As I see it, there are 4 options:
> 
> 1) Delete the insn completely, since it doesn't match, and then put in code
> later to cover the case when PR target/93230 is done.

Such a patch is pre-approved.

> 3) Patch the existing insns so that they do match, but don't add all of the
> other options that could be added (adding sign extension, adding the ability to
> load the value into vector registers with ISA 2.07, optimizing vec_extract
> being cnverted to floating point to avoid direct moves, etc.).
> 
> 4) Do all of the possibilities now.  The trouble is due to the number of
> special cases, this can add up to a number of new insns (for example, dealing
> with HImode/QImode also being zero extended to SImode in addition to DImode,
> dealing with QImode not having a sign extending load, dealing with SImode that
> can load into the vector registers at ISA 2.05/2.06 which HI/QI modes need
> 2.07, etc.).
> 
> Given we are in stage 4, I think #4 is not appropriate (but if you want, I can
> do the patches).

That goes for option 3 as well.

Thanks,


Segher

On Fri, Feb 28, 2020 at 06:45:25AM -0600, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Feb 28, 2020 at 12:32:06AM -0500, Michael Meissner wrote:
> > As part of my work in adding support for -mcpu=future, I noticed an insn that
> > would never match.
> 
> > It will never match, because the zero_extend result is the same mode as the
> > input, so the machine independent parts of the compiler would never insert the
> > zero extend.
> 
> It's not valid RTL, even:
>   @findex zero_extend
>   @item (zero_extend:@var{m} @var{x})
>   Represents the result of zero-extending the value @var{x}
>   to machine mode @var{m}.  @var{m} must be a fixed-point mode
>   and @var{x} a fixed-point value of a mode narrower than @var{m}.
> 
> > There is a wider issue to optimize all cases of vec_extract to do the sign,
> > zero, and float extension automatically when we are loading from memory, which
> > is PR target/93230.  I have patches for all of the cases for 93230, but they
> > will need to wait until GCC 11 opens up.
> 
> If you don't use reload_completed in the split condition you do not have
> this problem (in the normal case).  Please work on that?

No.  I tend to think that if we do the split before reload, that it will cause
some regressions, because the register allocator will take the opportunity to
change loads to vector registers to be loads to GPRs and direct moves.  One of
the original motivations for some of these patches is to avoid direct moves.

I also worry that things like having to use SUBREG's before RA (instead of just
changing the mode and/or the register number that we can do after reload) will
not work because generally vectors and scalars aren't tieable.

On Mon, Mar 02, 2020 at 07:41:42PM -0500, Michael Meissner wrote:
> On Fri, Feb 28, 2020 at 06:45:25AM -0600, Segher Boessenkool wrote:
> > On Fri, Feb 28, 2020 at 12:32:06AM -0500, Michael Meissner wrote:
> > > There is a wider issue to optimize all cases of vec_extract to do the sign,
> > > zero, and float extension automatically when we are loading from memory, which
> > > is PR target/93230.  I have patches for all of the cases for 93230, but they
> > > will need to wait until GCC 11 opens up.
> > 
> > If you don't use reload_completed in the split condition you do not have
> > this problem (in the normal case).  Please work on that?
> 
> No.  I tend to think that if we do the split before reload, that it will cause
> some regressions, because the register allocator will take the opportunity to
> change loads to vector registers to be loads to GPRs and direct moves.

It will cause better optimisations, yes.  Earlier optimisations.

The compiler will use a different register set if it thinks that is
cheaper to do.  We need to get that right *anyway*, because it is used
from many more places.

> One of
> the original motivations for some of these patches is to avoid direct moves.

You only need to do all of this manually because you split after reload.
That is only a good thing to do if you *have* to, usually because some
datum can end up in memory or in a register, and those are significantly
differently for the machine code you need.

This is not true here, because you do *not* allow both regs and mem (in
the normal case).  If you split earlier, you do not have to do all the
main rtl optimisations (say cprop, fwprop, combine, insn selection in
general) manually, as you do have to do to not get terrible code if you
split after reload.

> I also worry that things like having to use SUBREG's before RA (instead of just
> changing the mode and/or the register number that we can do after reload) will
> not work because generally vectors and scalars aren't tieable.

You have pseudos before reload.  Subregs work fine.

If they weren't tieable you could not do this on hard regs either, so I
don't see your point at all here?


Segher