[2/4] Make vsx_splat_<mode>_reg use correct insn attributes, PR target/99293

  Make vsx_splat_<mode>_reg use correct insn attributes, PR target/99293

In looking at PR target/99293, I noticed that the code in the insn
vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the
"mtvsrdd" is generated.  It should use "mfvsr".  I also added a "p9v" isa
attribute for that alternative.

I have built the spec 2017 benchmark with this patch (#2) and the next
patch (#3), along with the first patch in the series for power9 and
power10 targets.  Most of the floating point benchmarks changed code
slightly, due to the scheduling changes that came from changing the insn
type attribute.  I ran the spec 2017 suite on power9, and I did not not
notice any significant changes from these changes.

The power9 benchmarks that had code differences with these 2 patches
applied in addition to the build with just the first patch applied were:

	namd_r, pareset_r, povray_r, wrf_r, blender_r, cam4_r,
	deepsjeng_r, imagick_r, roms_r

The power9 benchmarks that had code differences with these 2 patches
applied in addition to the build with just the first patch applied were
(cactuBSSN_r had changes for power10 but not power9):

	cactuBSSN_r, namd_r, pareset_r, povray_r, wrf_r, blender_r,
	cam4_r, deepsjeng_r, imagick_r, nab_r, roms_r

I have built bootstrap versions on the following systems.  There were no
regressions in the runs:

	Power9 little endian, --with-cpu=power9
	Power10 little endian, --with-cpu=power10
	Power8 big endian, --with-cpu=power8 (both 32-bit & 64-bit tests)

Can I install this into the trunk?  After a burn-in period, can I backport
and install this into GCC 11 and GCC 10 branches?

2022-03-28   Michael Meissner  <meissner@linux.ibm.com>

gcc/
	PR target/99293
	* config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct
	insn type attribute.  Add "p9v" isa attribute for generating the
	mtvsrdd instruction.
---
 gcc/config/rs6000/vsx.md | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

Hi!

On Mon, Mar 28, 2022 at 12:27:05PM -0400, Michael Meissner wrote:
> In looking at PR target/99293, I noticed that the code in the insn
> vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the
> "mtvsrdd" is generated.  It should use "mfvsr".  I also added a "p9v" isa
> attribute for that alternative.

s/mfvsr/mtvsr/

But, mtvsrd and mtvsrdd have very different scheduling properties (like,
on p10 it is 1 cycle vs. 3 cycles).

Also, there are two insn patterns for mtvsrdd, and you are only touching
one here.

> 	* config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct
> 	insn type attribute.  Add "p9v" isa attribute for generating the
> 	mtvsrdd instruction.

This is in vsx.md, instead.

> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg"
>    "@
>     xxpermdi %x0,%x1,%x1,0
>     mtvsrdd %x0,%1,%1"
> -  [(set_attr "type" "vecperm,vecmove")])
> +  [(set_attr "type" "vecperm,mtvsr")
> +   (set_attr "isa" "*,p9v")])

"we" requires "p9v".  Please do a full conversion when getting rid of
this?  That includes requiring TARGET_POWERPC64 for it (not -m64 as its
documentation says; the existing implementation of "we" is correct).


Segher
  

On Mon, Mar 28, 2022 at 03:28:39PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Mar 28, 2022 at 12:27:05PM -0400, Michael Meissner wrote:
> > In looking at PR target/99293, I noticed that the code in the insn
> > vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the
> > "mtvsrdd" is generated.  It should use "mfvsr".  I also added a "p9v" isa
> > attribute for that alternative.
> 
> s/mfvsr/mtvsr/
> 
> But, mtvsrd and mtvsrdd have very different scheduling properties (like,
> on p10 it is 1 cycle vs. 3 cycles).


I must admit, I assumed vecmove was a stand-in for XXMR (i.e. XXLOR).  Since
its use is used for other cases (mtvsrdd, xxsel/vsel, x{s,v}abs*, x{s,v}nabs*,
xsiexpq*), it is probably better to just let things lie, and perhaps relook at
it in the GCC 13 time frame.
 
> Also, there are two insn patterns for mtvsrdd, and you are only touching
> one here.

I think you meant that comment about the third patch (to vsx_extract_<mode>)
and not to this patch (to vsx_splat_<mode>_reg) where there are only two
alternatives (the first being xxpermdi and the second being mtvsrdd).

> > 	* config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct
> > 	insn type attribute.  Add "p9v" isa attribute for generating the
> > 	mtvsrdd instruction.
> 
> This is in vsx.md, instead.
> 
> > --- a/gcc/config/rs6000/vsx.md
> > +++ b/gcc/config/rs6000/vsx.md
> > @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg"
> >    "@
> >     xxpermdi %x0,%x1,%x1,0
> >     mtvsrdd %x0,%1,%1"
> > -  [(set_attr "type" "vecperm,vecmove")])
> > +  [(set_attr "type" "vecperm,mtvsr")
> > +   (set_attr "isa" "*,p9v")])
> 
> "we" requires "p9v".  Please do a full conversion when getting rid of
> this?  That includes requiring TARGET_POWERPC64 for it (not -m64 as its
> documentation says; the existing implementation of "we" is correct).

That is more complex, and likely it should be a GCC 13 thing.  Off the top of
my head, we would need a new "isa" variant (p9v64) that combines p9v and
64-bit.  Originally, I had changed the "we" to "wa", but then I realized it
wouldn't work for 32-bit, but I left in setting the alternative.
  

On Wed, Mar 30, 2022 at 06:41:59PM -0400, Michael Meissner wrote:
> On Mon, Mar 28, 2022 at 03:28:39PM -0500, Segher Boessenkool wrote:
> > On Mon, Mar 28, 2022 at 12:27:05PM -0400, Michael Meissner wrote:
> > > In looking at PR target/99293, I noticed that the code in the insn
> > > vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the
> > > "mtvsrdd" is generated.  It should use "mfvsr".  I also added a "p9v" isa
> > > attribute for that alternative.
> > 
> > s/mfvsr/mtvsr/
> > 
> > But, mtvsrd and mtvsrdd have very different scheduling properties (like,
> > on p10 it is 1 cycle vs. 3 cycles).
> 
> I must admit, I assumed vecmove was a stand-in for XXMR (i.e. XXLOR).

That is "veclogical".  I don't think there is any core where this is
optimised specially?

> Since
> its use is used for other cases (mtvsrdd, xxsel/vsel, x{s,v}abs*, x{s,v}nabs*,
> xsiexpq*), it is probably better to just let things lie, and perhaps relook at
> it in the GCC 13 time frame.

Yes, we need to make better categories.  The problem is to come up with
something that is close enough to what the relevant cores actually do,
but in such a way that we do not end up with gazillions of nonsensical
separate instruction types.

What we care about most for p9 and p10 vector insns is whether something
is a 3-cycle op or not.  But this differs per core, and in ways that
are a little ad-hoc (looked at from far away anyway).

For the integer insns we ended up with extra attributes (not just
"type"), which is both compact and expressive.  We should try to do
something like that for vector ops as well.  We now have both p9 an
p10, with two implementations it should be clearer what a good direction
to take will be here.

> > Also, there are two insn patterns for mtvsrdd, and you are only touching
> > one here.
> 
> I think you meant that comment about the third patch (to vsx_extract_<mode>)
> and not to this patch (to vsx_splat_<mode>_reg) where there are only two
> alternatives (the first being xxpermdi and the second being mtvsrdd).

I mean vsx_concat_<mode> and vsx_splat_<mode>_reg.  Both have mtvsrdd
(both as alternative 1), but you only update the "type" of the latter
here.

> > > --- a/gcc/config/rs6000/vsx.md
> > > +++ b/gcc/config/rs6000/vsx.md
> > > @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg"
> > >    "@
> > >     xxpermdi %x0,%x1,%x1,0
> > >     mtvsrdd %x0,%1,%1"
> > > -  [(set_attr "type" "vecperm,vecmove")])
> > > +  [(set_attr "type" "vecperm,mtvsr")
> > > +   (set_attr "isa" "*,p9v")])
> > 
> > "we" requires "p9v".  Please do a full conversion when getting rid of
> > this?  That includes requiring TARGET_POWERPC64 for it (not -m64 as its
> > documentation says; the existing implementation of "we" is correct).
> 
> That is more complex, and likely it should be a GCC 13 thing.

Yes.

> Off the top of
> my head, we would need a new "isa" variant (p9v64) that combines p9v and
> 64-bit.

Not at all no.  Things that *use* the "isa" attribute can use other
attributes as well, if they want.  The reason we have "p9v" is because
it is so common that a shorthand helps (and *all* p9 vector insns need
either it or separate stuff).

> Originally, I had changed the "we" to "wa", but then I realized it
> wouldn't work for 32-bit, but I left in setting the alternative.

Yeah, when I got rid of many of the w* things I left mostly the harder
ones for later.  Sorry!


Segher