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

Message ID 20210217171730.GA11569@ibm-toto.the-meissners.org
State New
Headers show
Series
  • PR 99133, Mark xxspltiw, xxspltidp, and xxsplti32x as being prefixed
Related show

Commit Message

Marek Polacek via Gcc-patches Feb. 17, 2021, 5:17 p.m.
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(-)

-- 
2.22.0


-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Comments

Segher Boessenkool Feb. 18, 2021, 12:09 a.m. | #1
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
Marek Polacek via Gcc-patches Feb. 19, 2021, 3:44 a.m. | #2
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.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797
Segher Boessenkool Feb. 19, 2021, 9:15 p.m. | #3
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

Patch

diff --git a/gcc/config/rs6000/altivec.md b/gcc/config/rs6000/altivec.md
index 27a269b9e72..d409acce3a9 100644
--- a/gcc/config/rs6000/altivec.md
+++ b/gcc/config/rs6000/altivec.md
@@ -826,7 +826,8 @@  (define_insn "xxspltiw_v4si"
 		     UNSPEC_XXSPLTIW))]
  "TARGET_POWER10"
  "xxspltiw %x0,%1"
- [(set_attr "type" "vecsimple")])
+ [(set_attr "type" "vecsimple")
+  (set_attr "prefixed" "special")])
 
 (define_expand "xxspltiw_v4sf"
   [(set (match_operand:V4SF 0 "register_operand" "=wa")
@@ -845,7 +846,8 @@  (define_insn "xxspltiw_v4sf_inst"
 		     UNSPEC_XXSPLTIW))]
  "TARGET_POWER10"
  "xxspltiw %x0,%1"
- [(set_attr "type" "vecsimple")])
+ [(set_attr "type" "vecsimple")
+  (set_attr "prefixed" "special")])
 
 (define_expand "xxspltidp_v2df"
   [(set (match_operand:V2DF 0 "register_operand" )
@@ -864,7 +866,8 @@  (define_insn "xxspltidp_v2df_inst"
 		     UNSPEC_XXSPLTID))]
   "TARGET_POWER10"
   "xxspltidp %x0,%1"
-  [(set_attr "type" "vecsimple")])
+  [(set_attr "type" "vecsimple")
+  (set_attr "prefixed" "special")])
 
 (define_expand "xxsplti32dx_v4si"
   [(set (match_operand:V4SI 0 "register_operand" "=wa")
@@ -883,7 +886,8 @@  (define_expand "xxsplti32dx_v4si"
 					 GEN_INT (index), operands[3]));
    DONE;
 }
- [(set_attr "type" "vecsimple")])
+ [(set_attr "type" "vecsimple")
+  (set_attr "prefixed" "special")])
 
 (define_insn "xxsplti32dx_v4si_inst"
   [(set (match_operand:V4SI 0 "register_operand" "=wa")
@@ -893,7 +897,8 @@  (define_insn "xxsplti32dx_v4si_inst"
 		     UNSPEC_XXSPLTI32DX))]
   "TARGET_POWER10"
   "xxsplti32dx %x0,%2,%3"
-  [(set_attr "type" "vecsimple")])
+  [(set_attr "type" "vecsimple")
+  (set_attr "prefixed" "special")])
 
 (define_expand "xxsplti32dx_v4sf"
   [(set (match_operand:V4SF 0 "register_operand" "=wa")
@@ -921,7 +926,8 @@  (define_insn "xxsplti32dx_v4sf_inst"
 		     UNSPEC_XXSPLTI32DX))]
   "TARGET_POWER10"
   "xxsplti32dx %x0,%2,%3"
-  [(set_attr "type" "vecsimple")])
+  [(set_attr "type" "vecsimple")
+  (set_attr "prefixed" "special")])
 
 (define_insn "xxblend_<mode>"
   [(set (match_operand:VM3 0 "register_operand" "=wa")
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 798a715005c..f01fda35459 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -22031,7 +22031,7 @@  rs6000_insn_cost (rtx_insn *insn, bool speed)
   if (n == 0)
     {
       int length = get_attr_length (insn);
-      if (get_attr_prefixed (insn) == PREFIXED_YES)
+      if (get_attr_prefixed (insn) != PREFIXED_NO)
 	{
 	  int adjust = 0;
 	  ADJUST_INSN_LENGTH (insn, adjust);
@@ -26212,11 +26212,16 @@  static bool next_insn_prefixed_p;
    insn is a prefixed insn where we need to emit a 'p' before the insn.
 
    In addition, if the insn is part of a PC-relative reference to an external
-   label optimization, this is recorded also.  */
+   label optimization, this is recorded also.
+
+   Some prefixed instructions (xxspltiw, xxspltidp, xxsplti32dp, etc.) do not
+   have a leading 'p'.  Setting the prefix attribute to special does not the 'p'
+   prefix.  */
 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;
 }
 
@@ -26253,7 +26258,7 @@  rs6000_adjust_insn_length (rtx_insn *insn, int length)
     {
       rtx pattern = PATTERN (insn);
       if (GET_CODE (pattern) != USE && GET_CODE (pattern) != CLOBBER
-	  && get_attr_prefixed (insn) == PREFIXED_YES)
+	  && get_attr_prefixed (insn) != PREFIXED_NO)
 	{
 	  int num_prefixed = get_attr_max_prefixed_insns (insn);
 	  length += 4 * (num_prefixed + 1);
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index d343db0261e..0579e3be71a 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -270,7 +270,12 @@  (define_attr "cannot_copy" "no,yes" (const_string "no"))
 ;; 34 bits.  The macro ASM_OUTPUT_OPCODE emits a leading 'p' for prefixed
 ;; insns.  The default "length" attribute will also be adjusted by default to
 ;; be 12 bytes.
-(define_attr "prefixed" "no,yes"
+;;
+;; Normally rs6000_final_prescan_insn will add an initial 'p' before the
+;; instruction.  Some prefixed instructions (xxspltiw, xxspltidp, xxsplti32dp,
+;; etc.) do not have a leading 'p'.  If you set prefixed to special, it does
+;; not add this initial 'p'.
+(define_attr "prefixed" "no,yes,special"
   (cond [(ior (match_test "!TARGET_PREFIXED")
 	      (match_test "!NONJUMP_INSN_P (insn)"))
 	 (const_string "no")