[PATCH, rs6000] Update instruction attributes for Power10

Segher Boessenkool segher@kernel.crashing.org
Mon Nov 9 21:54:40 GMT 2020


On Wed, Nov 04, 2020 at 02:42:39PM -0600, Pat Haugen wrote:
> This patch updates the type/prefixed/dot/size attributes for various new instructions (and a couple existing that were incorrect) in preparation for the Power10 scheduling patch that will be following.

> --- a/gcc/config/rs6000/altivec.md
> +++ b/gcc/config/rs6000/altivec.md
> @@ -819,7 +819,7 @@ (define_insn "vs<SLDB_lr>db_<mode>"
>  	      VSHIFT_DBL_LR))]
>    "TARGET_POWER10"
>    "vs<SLDB_lr>dbi %0,%1,%2,%3"
> -  [(set_attr "type" "vecsimple")])
> +  [(set_attr "type" "vecperm")])

Is that such a good type for this?  I know the vsl etc. insns use it as
well, but :-)

These insns use the PM pipe on p9, which is documented as "Permute /
128b FX".  So maybe something like that can be a better name?

(Just food for thought.)

> @@ -827,7 +827,8 @@ (define_insn "xxspltiw_v4si"
>  		     UNSPEC_XXSPLTIW))]
>   "TARGET_POWER10"
>   "xxspltiw %x0,%1"
> - [(set_attr "type" "vecsimple")])
> + [(set_attr "type" "vecperm")
> +  (set_attr "prefixed" "always")])

Like I said in the other thread, you could have a "maybe_prefixed"
attribute (which you use on pretty much all existing ones), and only if
that is set the "prefixed" attribute is set to "yes" or "no"
automatically.  And things like this that always want it set can just do
so.  The code that decides whether to prefix the mnemonic with a "p" can
then just look if "maybe_prefixed" is "yes".

I don't know what is nicer, such a scheme or what you have here.  Will
ran into a pitfall already, so dunno.

> @@ -1080,7 +1088,8 @@ (define_insn "vstril_p_code_<mode>"
>  		   UNSPEC_VSTRIR))]
>    "TARGET_POWER10"
>    "vstri<wd>l. %0,%1"
> -  [(set_attr "type" "vecsimple")])
> +  [(set_attr "type" "vecperm")
> +   (set_attr "dot" "yes")])

"dot" is documented as
;; Is this instruction record form ("dot", signed compare to 0, writing CR0)?
which this insn doesn't do (it doesn't do a comparison, and it writes to
CR6).  It is fine to have this attribute if that is useful, but then
change the docs?  (FP insns write CR1 btw; that is all three fields that
record form insns can write).

>  ;; Fused multiply subtract 
>  (define_insn "*altivec_vnmsubfp"
> @@ -2779,7 +2788,7 @@ (define_insn "altivec_lvsl_reg"
>  	UNSPEC_LVSL_REG))]
>    "TARGET_ALTIVEC"
>    "lvsl %0,0,%1"
> -  [(set_attr "type" "vecload")])
> +  [(set_attr "type" "vecperm")])

That is not correct on older processors (7400, 970, etc.), and it even
matters there (you can have only one insn per pipe in each cycle).  Not
that that will matter much for GCC 11, but perhaps we can make the model
better for new CPUs without making it worse for older ones :-)

> @@ -4465,7 +4475,7 @@ (define_insn "vcfuged"
>  	 UNSPEC_VCFUGED))]
>     "TARGET_POWER10"
>     "vcfuged %0,%1,%2"
> -   [(set_attr "type" "vecsimple")])
> +   [(set_attr "type" "crypto")])
>  
>  (define_insn "vclzdm"
>    [(set (match_operand:V2DI 0 "altivec_register_operand" "=v")
> @@ -4474,7 +4484,7 @@ (define_insn "vclzdm"
>  	 UNSPEC_VCLZDM))]
>     "TARGET_POWER10"
>     "vclzdm %0,%1,%2"
> -   [(set_attr "type" "vecsimple")])
> +   [(set_attr "type" "crypto")])

Yeah that is a pretty strange type for what these insn do.

We already have the name "veccomplex", but a name like that would be
fitting perhaps.

> @@ -345,4 +357,5 @@ (define_insn "dfp_dscri_<mode>"
>  		     UNSPEC_DSCRI))]
>    "TARGET_DFP"
>    "dscri<q> %0,%1,%2"
> -  [(set_attr "type" "dfp")])
> +  [(set_attr "type" "dfp")
> +   (set_attr "size" "<bits>")])

All the DFP size changes are fine.

> diff --git a/gcc/config/rs6000/mma.md b/gcc/config/rs6000/mma.md
> index a3fd28bdd0a..43d6b618929 100644
> --- a/gcc/config/rs6000/mma.md
> +++ b/gcc/config/rs6000/mma.md
> @@ -302,6 +302,7 @@ (define_insn_and_split "*movpoi"
>    DONE;
>  }
>    [(set_attr "type" "vecload,vecstore,veclogical")
> +   (set_attr "size" "256,256,*")
>     (set_attr "length" "*,*,8")])

And this, too.

> @@ -589,4 +599,5 @@ (define_insn "mma_<avvi4i4i4>"
>    "TARGET_MMA"
>    "<avvi4i4i4> %A0,%x2,%x3,%4,%5,%6"
>    [(set_attr "type" "mma")
> +   (set_attr "prefixed" "always")
>     (set_attr "length" "8")])

And all the prefixed ones, too.

> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 4d528a39a37..55c47140672 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -25752,7 +25752,7 @@ static bool next_insn_prefixed_p;
>  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_YES);
>    return;
>  }

Add a comment here?  "always" is not surprising in many places, but here
it is.

> -;; Whether an insn is a prefixed insn, and an initial 'p' should be printed
> -;; before the instruction.  A prefixed instruction has a prefix instruction
> -;; word that extends the immediate value of the instructions from 12-16 bits to
> -;; 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"
> +;; Whether an insn is a prefixed insn.  A prefixed instruction has a prefix
> +;; instruction word that extends the immediate value of the instructions from
> +;; 12-16 bits to 34 bits.  The macro ASM_OUTPUT_OPCODE emits a leading 'p' for
> +;; prefixed="yes" insns.  The default "length" attribute will also be adjusted
> +;; by default to be 12 bytes.

In xxspltiw it is only 32 bits, not 34.  This comment really only
describes the 8LS-form prefix insns.

> @@ -2517,7 +2517,7 @@ (define_insn "pextd"
>  		   UNSPEC_PEXTD))]
>     "TARGET_POWER10 && TARGET_POWERPC64"
>     "pextd %0,%1,%2"
> -   [(set_attr "type" "integer")])
> +   [(set_attr "type" "crypto")])

Same as above, but for integer instead of vector.

>  (define_insn "bswapdi2_reg"
> @@ -5242,7 +5242,7 @@ (define_insn "setbc_<un>signed_<GPR:mode>"
>  			 (const_int 0)]))]
>    "TARGET_POWER10"
>    "setbc %0,%j1"
> -  [(set_attr "type" "isel")])
> +  [(set_attr "type" "integer")])

Why?  Are these insns executed differently than the isel insn?  They do
take a CR field as input, which none of the integer isns do.

> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -2032,7 +2032,7 @@ (define_insn "*xvtlsbb_internal"
>  	 UNSPEC_XVTLSBB))]
>    "TARGET_POWER10"
>    "xvtlsbb %0,%x1"
> -  [(set_attr "type" "logical")])
> +  [(set_attr "type" "veccmp")])

Yup.

Maybe split the patch into a few themes?  Some themes are much easier
than others to review, are much more obvious; and they are all
independent anyway?


Segher


More information about the Gcc-patches mailing list