PR 99133, Mark xxspltiw, xxspltidp, and xxsplti32x as being prefixed

PR 99133, Mark xxspltiw, xxspltidp, and xxsplti32x as being prefixed

I noticed that the power10 xxspltiw, xxspltidp, and xxsplti32dx
instructions are not flagged as prefixed instructions, which means the
instruction length is not set to 12 bytes.  This patch sets these
instructions to be prefixed.  It also ensures that a leading 'p' is not
emitted before the instruction.

I checked this patch by doing a bootstrap build/check on a little endian power8
server system.  There were no regressions.  In addition, I debugged cc1, and I
put a breakpoint on the get_attr_length function and I verified the insns now
have length 12.  Can I check this patch into the master branch?

GCC 10 does not have support for these instructions, so I will not need to do a
backport.

gcc/
2021-02-17  Michael Meissner  <meissner@linux.ibm.com>

	PR target/99133
	* config/rs6000/altivec.md (xxspltiw_v4si): Set prefixed
	attribute.
	(xxspltiw_v4sf): Set prefixed attribute.
	(xxspltidp_v2df): Set prefixed attribute.
	(xxsplti32dx_v4si): Set prefixed attribute.
	(xxsplti32dx_v4si_inst): Set prefixed attribute.
	(xxsplti32dx_v4sf): Set prefixed attribute.
	* config/rs6000/rs6000.c (rs6000_insn_cost): Add support for
	setting prefixed attribute to special.
	(rs6000_final_prescan_insn): If prefixed attribute is special, do
	not emit a leading 'p' before the instruction.
	(rs6000_adjust_insn_length): Add support for setting prefixed
	attribute to special.
	* config/rs6000/rs6000.md (prefixed attribute): Add 'special' case
	for prefixed instructions that do not use a leading 'p'.
---
 gcc/config/rs6000/altivec.md | 18 ++++++++++++------
 gcc/config/rs6000/rs6000.c   | 13 +++++++++----
 gcc/config/rs6000/rs6000.md  |  7 ++++++-
 3 files changed, 27 insertions(+), 11 deletions(-)

Hi!

On Wed, Feb 17, 2021 at 12:17:30PM -0500, Michael Meissner wrote:
> I noticed that the power10 xxspltiw, xxspltidp, and xxsplti32dx
> instructions are not flagged as prefixed instructions, which means the
> instruction length is not set to 12 bytes.  This patch sets these
> instructions to be prefixed.  It also ensures that a leading 'p' is not
> emitted before the instruction.
> 
> I checked this patch by doing a bootstrap build/check on a little endian power8
> server system.  There were no regressions.

Why test a p10 patch on a p8?

> In addition, I debugged cc1, and I
> put a breakpoint on the get_attr_length function and I verified the insns now
> have length 12.

You can just use -dp; the generated assembler output has lines like
	pla 3,.LC0@pcrel         # 6    [c=4 l=12]  *pcrel_local_addr
(c is cost, l is length).

      fprintf (asm_out_file, "[c=%d",
               insn_cost (debug_insn, optimize_insn_for_speed_p ()));
      if (HAVE_ATTR_length)
        fprintf (asm_out_file, " l=%d",
                 get_attr_length (debug_insn));
      fprintf (asm_out_file, "]  ");


> +   Some prefixed instructions (xxspltiw, xxspltidp, xxsplti32dp, etc.) do not
> +   have a leading 'p'.  Setting the prefix attribute to special does not the 'p'
> +   prefix.  */

(grammar)

"special" is the *normal* case.  *Most* prefixed insns will be like
this.  They aren't right now, but they will be.

It sounds like you should make a new attribute, "always_prefixed" or
something, that then the code that sets "prefixed" can use.

>  void
>  rs6000_final_prescan_insn (rtx_insn *insn, rtx [], int)
>  {
> -  next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO);
> +  next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO
> +			  && get_attr_prefixed (insn) != PREFIXED_SPECIAL);
>    return;
>  }

So you set next_insn_prefixed_p when exactly?  The original code was
correct, as far as I can see?


There are four kinds of insns now:

1) Never prefixed.
2) Always prefixed, like xxspltiw but many more in the future.
3) Sometimes prefixed, and they get a "p" mnemonic prefix then.  Those
are the majority currently, since we mostly have load/store insns that
are prefixed currently, and those usually have a non-prefixed form we
handled already.
4) Sometimes prefixed, and they get a "pm" prefix then.  We don't handle
those yet (those are the MMA GER insns).

The "prefixed" attribute should just mean if the instruction ended up as
some prefixed form (8 bytes).

So, for insns like xxspltiw you should just set "prefixed" to "yes",
because that makes sense, is not confusing.  The code that prefixes "p"
to the mnemonic should change, instead.  It can look at some new
attribute, but it could also just use
  prefixed_load_p (insn) || prefixed_store_p (insn)
  || prefixed_paddi_p (insn)
or similar (perhaps make a helper function for that then?)

Thanks,


Segher

On Wed, Feb 17, 2021 at 06:09:39PM -0600, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Feb 17, 2021 at 12:17:30PM -0500, Michael Meissner wrote:
> > I noticed that the power10 xxspltiw, xxspltidp, and xxsplti32dx
> > instructions are not flagged as prefixed instructions, which means the
> > instruction length is not set to 12 bytes.  This patch sets these
> > instructions to be prefixed.  It also ensures that a leading 'p' is not
> > emitted before the instruction.
> > 
> > I checked this patch by doing a bootstrap build/check on a little endian power8
> > server system.  There were no regressions.
> 
> Why test a p10 patch on a p8?

Well it is just a code gen patch, so I can test it on any system, even an
x86_64 with a cross compiler.  Given that right now xxsplatiw is only created
via a built-in, The main consequence of not setting the prefixed attribute is
that the insn length is wrong.  Normal functions probably won't even notice,
but it is certainly possible that some function is large enough and it uses
xxsplatiw (and friends) and the assembler would complain that conditional jumps
are too far.

At the moment given it is only generated as a built-in function, I doubt people
would run into it.  As we add support in GCC 12 to use these instructions to
load up constants into the vector registers, it is more likely that people will
run into issues if the length is wrong.

> > In addition, I debugged cc1, and I
> > put a breakpoint on the get_attr_length function and I verified the insns now
> > have length 12.
> 
> You can just use -dp; the generated assembler output has lines like
> 	pla 3,.LC0@pcrel         # 6    [c=4 l=12]  *pcrel_local_addr
> (c is cost, l is length).
> 
>       fprintf (asm_out_file, "[c=%d",
>                insn_cost (debug_insn, optimize_insn_for_speed_p ()));
>       if (HAVE_ATTR_length)
>         fprintf (asm_out_file, " l=%d",
>                  get_attr_length (debug_insn));
>       fprintf (asm_out_file, "]  ");

Sure, but it is just as simple to verify it with a debugger.

> > +   Some prefixed instructions (xxspltiw, xxspltidp, xxsplti32dp, etc.) do not
> > +   have a leading 'p'.  Setting the prefix attribute to special does not the 'p'
> > +   prefix.  */
> 
> (grammar)
> 
> "special" is the *normal* case.  *Most* prefixed insns will be like
> this.  They aren't right now, but they will be.
> 
> It sounds like you should make a new attribute, "always_prefixed" or
> something, that then the code that sets "prefixed" can use.

Yes agreed.

> >  void
> >  rs6000_final_prescan_insn (rtx_insn *insn, rtx [], int)
> >  {
> > -  next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO);
> > +  next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO
> > +			  && get_attr_prefixed (insn) != PREFIXED_SPECIAL);
> >    return;
> >  }
> 
> So you set next_insn_prefixed_p when exactly?  The original code was
> correct, as far as I can see?

rs6000_final_prescan_insn is called in the FINAL_PRESCAN_INSN target hook, and
it is the only place we can set the flag for ASM_OUTPUT_OPCODE to print the
initial 'p'.  As you know, during the development of prefixed support, I went
through various different methods of generating the prefixed instruction
(i.e. pld instead of ld).  The current method works for normal load, store and
add instructions that have a normal form and a prefixed form.  But it doesn't
work for other instructions that we need to start dealing with. 
> 
> There are four kinds of insns now:
> 
> 1) Never prefixed.
> 2) Always prefixed, like xxspltiw but many more in the future.
> 3) Sometimes prefixed, and they get a "p" mnemonic prefix then.  Those
> are the majority currently, since we mostly have load/store insns that
> are prefixed currently, and those usually have a non-prefixed form we
> handled already.
> 4) Sometimes prefixed, and they get a "pm" prefix then.  We don't handle
> those yet (those are the MMA GER insns).
> 
> The "prefixed" attribute should just mean if the instruction ended up as
> some prefixed form (8 bytes).
> 
> So, for insns like xxspltiw you should just set "prefixed" to "yes",
> because that makes sense, is not confusing.  The code that prefixes "p"
> to the mnemonic should change, instead.  It can look at some new
> attribute, but it could also just use
>   prefixed_load_p (insn) || prefixed_store_p (insn)
>   || prefixed_paddi_p (insn)
> or similar (perhaps make a helper function for that then?)

Yes.  Pat Haugen will be taking over the PR.

On Thu, Feb 18, 2021 at 10:44:14PM -0500, Michael Meissner wrote:
> On Wed, Feb 17, 2021 at 06:09:39PM -0600, Segher Boessenkool wrote:
> > Why test a p10 patch on a p8?
> 
> Well it is just a code gen patch, so I can test it on any system, even an
> x86_64 with a cross compiler.  Given that right now xxsplatiw is only created
> via a built-in,

That in itself should be fixed btw (don't use unspecs).

> > > +   Some prefixed instructions (xxspltiw, xxspltidp, xxsplti32dp, etc.) do not
> > > +   have a leading 'p'.  Setting the prefix attribute to special does not the 'p'
> > > +   prefix.  */
> > 
> > (grammar)
> > 
> > "special" is the *normal* case.  *Most* prefixed insns will be like
> > this.  They aren't right now, but they will be.
> > 
> > It sounds like you should make a new attribute, "always_prefixed" or
> > something, that then the code that sets "prefixed" can use.
> 
> Yes agreed.

Or better yet, "maybe_prefixed", which you set when something else will
make the decision.  Mmostly the default value of the prefixed attribute
will then do that. See "maybe_var_shift" / "var_shift" for example.

> > >  void
> > >  rs6000_final_prescan_insn (rtx_insn *insn, rtx [], int)
> > >  {
> > > -  next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO);
> > > +  next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO
> > > +			  && get_attr_prefixed (insn) != PREFIXED_SPECIAL);
> > >    return;
> > >  }
> > 
> > So you set next_insn_prefixed_p when exactly?  The original code was
> > correct, as far as I can see?
> 
> rs6000_final_prescan_insn is called in the FINAL_PRESCAN_INSN target hook, and
> it is the only place we can set the flag for ASM_OUTPUT_OPCODE to print the
> initial 'p'.  As you know, during the development of prefixed support, I went
> through various different methods of generating the prefixed instruction
> (i.e. pld instead of ld).  The current method works for normal load, store and
> add instructions that have a normal form and a prefixed form.  But it doesn't
> work for other instructions that we need to start dealing with. 

Ah, the flag doesn't mean that the next insn is prefixed.  It means we
want to prefix the mnemonic with a "p", instead.  So some name change is
in order (as a separate, trivial, patch at the start of the series) --
this helps bisection if needed later, but it also helps reviewing
enormously).

> > So, for insns like xxspltiw you should just set "prefixed" to "yes",
> > because that makes sense, is not confusing.  The code that prefixes "p"
> > to the mnemonic should change, instead.  It can look at some new
> > attribute, but it could also just use
> >   prefixed_load_p (insn) || prefixed_store_p (insn)
> >   || prefixed_paddi_p (insn)
> > or similar (perhaps make a helper function for that then?)
> 
> Yes.  Pat Haugen will be taking over the PR.

Thanks,


Segher