Re: [PATCH 2/2 ver 4] rs6000, fix vec_replace_unaligned built-in arguments

[Carl Love <cel@us.ibm.com>]

On Fri, 2023-07-21 at 13:04 +0800, Kewen.Lin wrote:
> Hi Carl,
> 
> on 2023/7/18 03:20, Carl Love wrote:
> > GCC maintainers:
> > 
> > Version 4, changed the new RS6000_OVLD_VEC_REPLACE_UN case
> > statement
> > rs6000/rs6000-c.cc.  The existing REPLACE_ELT iterator name was
> > changed
> > to REPLACE_ELT_V along with the associated
> > define_mode_attr.  Renamed
> > VEC_RU to REPLACE_ELT for the iterator name and VEC_RU_char to
> > REPLACE_ELT_char.  Fixed the double test in vec-replace-word-
> > runnable_1.c to be consistent with the other tests.  Removed the
> > "dg-do 
> > link" from both tests.  Put in an explicit cast in test vec-
> > replace-word-runnable_2.c to eliminate the need for the -flax-
> > vector-conversions dg-option.
> > 
> > Version 3, added code to altivec_resolve_overloaded_builtin so the
> > correct instruction is selected for the size of the second
> > argument. 
> > This restores the instruction counts to the original values where
> > the
> > correct instructions were originally being generated.  The naming
> > of
> > the overloaded builtin instances and builtin definitions were
> > changed
> > to reflect the type of the second argument since the type of the
> > first
> > argument is now the same for all overloaded instances.  A new
> > builtin
> > test file was added for the case where the first argument is cast
> > to
> > the unsigned long long type.  This test requires the -flax-vector-
> > conversions gcc command line option.  Since the other tests do not
> > require this option, I felt that the new test needed to be in a
> > separate file.  Finally some formatting fixes were made in the
> > original
> > test file.  Patch has been retested on Power 10 with no
> > regressions.
> > 
> > Version 2, fixed various typos.  Updated the change log body to say
> > the
> > instruction counts were updated.  The instruction counts changed as
> > a
> > result of changing the first argument of the vec_replace_unaligned
> > builtin call from vector unsigned long long (vull) to vector
> > unsigned
> > char (vuc).  When the first argument was vull the builtin call
> > generated the vinsd instruction for the two test cases.  The
> > updated
> > call with vuc as the first argument generates two vinsw
> > instructions
> > instead.  Patch was retested on Power 10 with no regressions.
> > 
> > The following patch fixes the first argument in the builtin
> > definition
> > and the corresponding test cases.  Initially, the builtin
> > specification
> > was wrong due to a cut and past error.  The documentation was fixed
> > in:
> > 
> >    commit ed3fea09b18f67e757b5768b42cb6e816626f1db
> >    Author: Bill Schmidt <wschmidt@linux.ibm.com>
> >    Date:   Fri Feb 4 13:07:17 2022 -0600
> > 
> >        rs6000: Correct function prototypes for
> > vec_replace_unaligned
> > 
> >        Due to a pasto error in the documentation,
> > vec_replace_unaligned was
> >        implemented with the same function prototypes as
> > vec_replace_elt.  
> >        It was intended that vec_replace_unaligned always specify
> > output
> >        vectors as having type vector unsigned char, to emphasize
> > that 
> >        elements are potentially misaligned by this built-in
> > function.  
> >        This patch corrects the misimplementation.
> >     ....
> > 
> > This patch fixes the arguments in the definitions and updates the
> > testcases accordingly.  Additionally, a few minor spacing issues
> > are
> > fixed.
> > 
> > The patch has been tested on Power 10 with no regressions.  Please
> > let
> > me know if the patch is acceptable for mainline.  Thanks.
> > 
> >                  Carl 
> > 
> > 
> > ----------------------------
> > rs6000, fix vec_replace_unaligned built-in arguments
> > 
> > The first argument of the vec_replace_unaligned built-in should
> > always be
> > of type unsigned char, as specified in gcc/doc/extend.texi.
> 
> Shouldn't be "vector unsigned char" instead of "unsigned char"?
> 
> Or do I miss something?

Nope, I missed saying "vector".  Fixed.

> 
> > This patch fixes the builtin definitions and updates the test cases
> > to use
> > the correct arguments.  The original test file is renamed and a
> > second test
> > file is added for a new test case.
> > 
> > gcc/ChangeLog:
> > 	* config/rs6000/rs6000-builtins.def: Rename
> > 	__builtin_altivec_vreplace_un_uv2di as
> > __builtin_altivec_vreplace_un_udi
> > 	__builtin_altivec_vreplace_un_uv4si as
> > __builtin_altivec_vreplace_un_usi
> > 	__builtin_altivec_vreplace_un_v2df as
> > __builtin_altivec_vreplace_un_df
> > 	__builtin_altivec_vreplace_un_v2di as
> > __builtin_altivec_vreplace_un_di
> > 	__builtin_altivec_vreplace_un_v4sf as
> > __builtin_altivec_vreplace_un_sf
> > 	__builtin_altivec_vreplace_un_v4si as
> > __builtin_altivec_vreplace_un_si.
> > 	Rename VREPLACE_UN_UV2DI as VREPLACE_UN_UDI, VREPLACE_UN_UV4SI
> > as
> > 	VREPLACE_UN_USI, VREPLACE_UN_V2DF as VREPLACE_UN_DF,
> > 	VREPLACE_UN_V2DI as VREPLACE_UN_DI, VREPLACE_UN_V4SF as
> > 	VREPLACE_UN_SF, VREPLACE_UN_V4SI as VREPLACE_UN_SI.
> > 	Rename vreplace_un_v2di as vreplace_un_di, vreplace_un_v4si as
> > 	vreplace_un_si, vreplace_un_v2df as vreplace_un_df,
> > 	vreplace_un_v2di as vreplace_un_di, vreplace_un_v4sf as
> > 	vreplace_un_sf, vreplace_un_v4si as vreplace_un_si.
> > 	* config/rs6000/rs6000-c.cc (find_instance): Add case
> > 	RS6000_OVLD_VEC_REPLACE_UN.
> > 	* config/rs6000/rs6000-overload.def (__builtin_vec_replace_un):
> > 	Fix first argument type.  Rename VREPLACE_UN_UV4SI as
> > 	VREPLACE_UN_USI, VREPLACE_UN_V4SI as VREPLACE_UN_SI,
> > 	VREPLACE_UN_UV2DI as VREPLACE_UN_UDI, VREPLACE_UN_V2DI as
> > 	VREPLACE_UN_DI, VREPLACE_UN_V4SF as VREPLACE_UN_SF,
> > 	VREPLACE_UN_V2DF as VREPLACE_UN_DF.
> > 	* config/rs6000/vsx.md (REPLACE_ELT): Renamed the mode_iterator
> > 	REPLACE_ELT_V.
> > 	(REPLACE_ELT_char): Renamed REPLACE_ELT_V_char.
> > 	(REPLACE_ELT_sh): Renamed REPLACE_ELT_V_sh.
> > 	(REPLACE_ELT_max): Renamed REPLACE_ELT_V_max.
> > 	(REPLACE_ELT): New mode iterator.
> > 	(REPLACE_ELT_char): New mode attribute.
> > 	(vreplace_un_<mode>): Change iterator and mode attribute.
> > 
> > gcc/testsuite/ChangeLog:
> > 	* gcc.target/powerpc/vec-replace-word-runnable.c: Renamed
> > 	vec-replace-word-runnable_1.c.
> > 	* gcc.target/powerpc/vec-replace-word-runnable_1.c
> > 	(dg-options): add -flax-vector-conversions.
> > 	(vec_replace_unaligned) Fix first argument type.
> > 	(vresult_uchar): Fix expected results.
> > 	(vec_replace_unaligned): Update for loop to check uchar
> > results.
> > 	Remove extra spaces in if statements. Insert missing spaces in
> > 	for statements.
> > 	* gcc.target/powerpc/vec-replace-word-runnable_2.c: New test
> > file.
> > ---
> >  gcc/config/rs6000/rs6000-builtins.def         |  28 +--
> >  gcc/config/rs6000/rs6000-c.cc                 |  23 +++
> >  gcc/config/rs6000/rs6000-overload.def         |  24 +--
> >  gcc/config/rs6000/vsx.md                      |  52 ++---
> >  ...nnable.c => vec-replace-word-runnable_1.c} | 177 ++++++++++--
> > ------
> >  .../powerpc/vec-replace-word-runnable_2.c     |  50 +++++
> >  6 files changed, 224 insertions(+), 130 deletions(-)
> >  rename gcc/testsuite/gcc.target/powerpc/{vec-replace-word-
> > runnable.c => vec-replace-word-runnable_1.c} (51%)
> >  create mode 100644 gcc/testsuite/gcc.target/powerpc/vec-replace-
> > word-runnable_2.c
> > 
> > diff --git a/gcc/config/rs6000/rs6000-builtins.def
> > b/gcc/config/rs6000/rs6000-builtins.def
> > index c0ef717161f..58c53cd462e 100644
> > --- a/gcc/config/rs6000/rs6000-builtins.def
> > +++ b/gcc/config/rs6000/rs6000-builtins.def
> > @@ -3397,26 +3397,26 @@
> >    const vull __builtin_altivec_vpextd (vull, vull);
> >      VPEXTD vpextd {}
> >  
> > -  const vuc __builtin_altivec_vreplace_un_uv2di (vull, unsigned
> > long long, \
> > -                                                 const int<4>);
> > -    VREPLACE_UN_UV2DI vreplace_un_v2di {}
> > +  const vuc __builtin_altivec_vreplace_un_udi (vuc, unsigned long
> > long, \
> > +                                               const int<4>);
> > +    VREPLACE_UN_UDI vreplace_un_di {}
> >  
> > -  const vuc __builtin_altivec_vreplace_un_uv4si (vui, unsigned
> > int, \
> > -                                                 const int<4>);
> > -    VREPLACE_UN_UV4SI vreplace_un_v4si {}
> > +  const vuc __builtin_altivec_vreplace_un_usi (vuc, unsigned int,
> > \
> > +                                               const int<4>);
> > +    VREPLACE_UN_USI vreplace_un_si {}
> >  
> > -  const vuc __builtin_altivec_vreplace_un_v2df (vd, double, const
> > int<4>);
> > -    VREPLACE_UN_V2DF vreplace_un_v2df {}
> > +  const vuc __builtin_altivec_vreplace_un_df (vuc, double, const
> > int<4>);
> > +    VREPLACE_UN_DF vreplace_un_df {}
> >  
> > -  const vuc __builtin_altivec_vreplace_un_v2di (vsll, signed long
> > long, \
> > +  const vuc __builtin_altivec_vreplace_un_di (vuc, signed long
> > long, \
> >                                                  const int<4>);
> > -    VREPLACE_UN_V2DI vreplace_un_v2di {}
> > +    VREPLACE_UN_DI vreplace_un_di {}
> >  
> > -  const vuc __builtin_altivec_vreplace_un_v4sf (vf, float, const
> > int<4>);
> > -    VREPLACE_UN_V4SF vreplace_un_v4sf {}
> > +  const vuc __builtin_altivec_vreplace_un_sf (vuc, float, const
> > int<4>);
> > +    VREPLACE_UN_SF vreplace_un_sf {}
> >  
> > -  const vuc __builtin_altivec_vreplace_un_v4si (vsi, signed int,
> > const int<4>);
> > -    VREPLACE_UN_V4SI vreplace_un_v4si {}
> > +  const vuc __builtin_altivec_vreplace_un_si (vuc, signed int,
> > const int<4>);
> > +    VREPLACE_UN_SI vreplace_un_si {}
> >  
> >    const vull __builtin_altivec_vreplace_uv2di (vull, unsigned long
> > long, \
> >                                                 const int<1>);
> > diff --git a/gcc/config/rs6000/rs6000-c.cc
> > b/gcc/config/rs6000/rs6000-c.cc
> > index 350987b851b..63ab360d352 100644
> > --- a/gcc/config/rs6000/rs6000-c.cc
> > +++ b/gcc/config/rs6000/rs6000-c.cc
> > @@ -1968,6 +1968,29 @@ altivec_resolve_overloaded_builtin
> > (location_t loc, tree fndecl,
> >  	      instance_code = RS6000_BIF_VSIEDP;
> >  	  }
> >  
> > +	tree call = find_instance (&unsupported_builtin, &instance,
> > +				   instance_code, fcode, types, args,
> > nargs);
> > +	if (call != error_mark_node)
> > +	  return call;
> > +	break;
> > +      }
> > +    case RS6000_OVLD_VEC_REPLACE_UN:
> > +      {
> > +	machine_mode arg2_mode = TYPE_MODE (types[1]);
> > +
> > +	if (arg2_mode == SImode)
> > +	  /* Signed and unsigned are handled the same.  */
> > +	  instance_code = RS6000_BIF_VREPLACE_UN_USI;
> > +	else if (arg2_mode == SFmode)
> > +	  instance_code = RS6000_BIF_VREPLACE_UN_SF;
> > +	else if (arg2_mode == DImode)
> > +	  /* Signed and unsigned are handled the same.  */
> > +	  instance_code = RS6000_BIF_VREPLACE_UN_UDI;
> > +	else if (arg2_mode == DFmode)
> > +	  instance_code = RS6000_BIF_VREPLACE_UN_DF;
> > +	else
> > +	  break;
> > +
> >  	tree call = find_instance (&unsupported_builtin, &instance,
> >  				   instance_code, fcode, types, args,
> > nargs);
> >  	if (call != error_mark_node)
> > diff --git a/gcc/config/rs6000/rs6000-overload.def
> > b/gcc/config/rs6000/rs6000-overload.def
> > index 470d718efde..b83946f5ad8 100644
> > --- a/gcc/config/rs6000/rs6000-overload.def
> > +++ b/gcc/config/rs6000/rs6000-overload.def
> > @@ -3059,18 +3059,18 @@
> >      VREPLACE_ELT_V2DF
> >  
> >  [VEC_REPLACE_UN, vec_replace_unaligned, __builtin_vec_replace_un]
> > -  vuc __builtin_vec_replace_un (vui, unsigned int, const int);
> > -    VREPLACE_UN_UV4SI
> > -  vuc __builtin_vec_replace_un (vsi, signed int, const int);
> > -    VREPLACE_UN_V4SI
> > -  vuc __builtin_vec_replace_un (vull, unsigned long long, const
> > int);
> > -    VREPLACE_UN_UV2DI
> > -  vuc __builtin_vec_replace_un (vsll, signed long long, const
> > int);
> > -    VREPLACE_UN_V2DI
> > -  vuc __builtin_vec_replace_un (vf, float, const int);
> > -    VREPLACE_UN_V4SF
> > -  vuc __builtin_vec_replace_un (vd, double, const int);
> > -    VREPLACE_UN_V2DF
> > +  vuc __builtin_vec_replace_un (vuc, unsigned int, const int);
> > +    VREPLACE_UN_USI
> > +  vuc __builtin_vec_replace_un (vuc, signed int, const int);
> > +    VREPLACE_UN_SI
> > +  vuc __builtin_vec_replace_un (vuc, unsigned long long, const
> > int);
> > +    VREPLACE_UN_UDI
> > +  vuc __builtin_vec_replace_un (vuc, signed long long, const int);
> > +    VREPLACE_UN_DI
> > +  vuc __builtin_vec_replace_un (vuc, float, const int);
> > +    VREPLACE_UN_SF
> > +  vuc __builtin_vec_replace_un (vuc, double, const int);
> > +    VREPLACE_UN_DF
> >  
> >  [VEC_REVB, vec_revb, __builtin_vec_revb]
> >    vss __builtin_vec_revb (vss);
> > diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> > index 0c269e4e8d9..d76f9f4864e 100644
> > --- a/gcc/config/rs6000/vsx.md
> > +++ b/gcc/config/rs6000/vsx.md
> > @@ -381,13 +381,20 @@ (define_int_attr
> > xvcvbf16       [(UNSPEC_VSX_XVCVSPBF16 "xvcvspbf16")
> >  (define_mode_iterator VI2 [V4SI V8HI V16QI V2DI])
> >  
> >  ;; Vector extract_elt iterator/attr for 32-bit and 64-bit elements

Missed this earlier, I think the comment needs fixing as well since we
now have both vector and scalar now.    Changed to:

;; Vector and scalar extract_elt iterator/attr for 32-bit and 64-bit
elements

> > -(define_mode_iterator REPLACE_ELT [V4SI V4SF V2DI V2DF])
> > -(define_mode_attr REPLACE_ELT_char [(V4SI "w") (V4SF "w")
> > -				    (V2DI  "d") (V2DF "d")])
> > -(define_mode_attr REPLACE_ELT_sh [(V4SI "2") (V4SF "2")
> > -				  (V2DI  "3") (V2DF "3")])
> > -(define_mode_attr REPLACE_ELT_max [(V4SI "12") (V4SF "12")
> > -				   (V2DI  "8") (V2DF "8")])
> > +(define_mode_iterator REPLACE_ELT_V [V4SI V4SF V2DI V2DF])
> > +(define_mode_attr REPLACE_ELT_V_char [(V4SI "w") (V4SF "w")
> > +				      (V2DI  "d") (V2DF "d")])
> > +(define_mode_attr REPLACE_ELT_V_sh [(V4SI "2") (V4SF "2")
> > +				    (V2DI  "3") (V2DF "3")])
> > +(define_mode_attr REPLACE_ELT_V_max [(V4SI "12") (V4SF "12")
> > +				     (V2DI  "8") (V2DF "8")])
> > +
> > +;; vector replace unaligned modes
> > +(define_mode_iterator REPLACE_ELT [SI SF DI DF])
> > +(define_mode_attr REPLACE_ELT_char [(SI "w")
> > +				    (SF "w")
> > +				    (DI "d")
> > +				    (DF "d")])
> 
> Sorry if I didn't describe it clearly, I actually adviced that:

I think maybe it got lost in the first attempt to change this which
didn't work.  Fixed it.
> 
> (define_mode_iterator REPLACE_ELT_V [V4SI V4SF V2DI V2DF])
> (define_mode_iterator REPLACE_ELT [SI SF DI DF])
> (define_mode_attr REPLACE_ELT_char [(V4SI "w") (V4SF "w")
> 				    (V2DI  "d") (V2DF "d")
> 				    (SI "w") (SF "w")
> 				    (DI  "d") (DF "d")])
> 
Having a single mode_attr for scalar and vector is a bit cleaner.


> It leaves REPLACE_ELT_sh and REPLACE_ELT_max unchanged, otherwise
> we have to update the preparation statements like what you did in
> this patch, IMHO both aligned and unaligned versions are replacing
> things with the element size, it's not quite meaningful to further
> distinguish the mode attributes.

OK, so I reverted REPLACE_ELT_V_sh back to REPLACE_ELT_sh and
REPLACE_ELT_V_max back to REPLACE_ELT_max.  Updated ChangeLog.

> 
> >  
> >  ;; Like VM2 in altivec.md, just do char, short, int, long, float
> > and double
> >  (define_mode_iterator VM3 [V4SI
> > @@ -4183,21 +4190,22 @@ (define_insn "vinsertgr_internal_<mode>"
> >   [(set_attr "type" "vecsimple")])
> >  
> >  (define_expand "vreplace_elt_<mode>"
> > -  [(set (match_operand:REPLACE_ELT 0 "register_operand")
> > -  (unspec:REPLACE_ELT [(match_operand:REPLACE_ELT 1
> > "register_operand")
> > -		       (match_operand:<VEC_base> 2 "register_operand")
> > -		       (match_operand:QI 3 "const_0_to_3_operand")]
> > -		      UNSPEC_REPLACE_ELT))]
> > +  [(set (match_operand:REPLACE_ELT_V 0 "register_operand")
> > +  (unspec:REPLACE_ELT_V [(match_operand:REPLACE_ELT_V 1
> > "register_operand")
> > +			 (match_operand:<VEC_base> 2
> > "register_operand")
> > +			 (match_operand:QI 3 "const_0_to_3_operand")]
> > +			UNSPEC_REPLACE_ELT))]
> >   "TARGET_POWER10"
> >  {
> >     int index;
> >     /* Immediate value is the word index, convert to byte index and
> > adjust for
> >        Endianness if needed.  */
> >     if (BYTES_BIG_ENDIAN)
> > -     index = INTVAL (operands[3]) << <REPLACE_ELT_sh>;
> > +     index = INTVAL (operands[3]) << <REPLACE_ELT_V_sh>;
> >  
> >     else
> > -     index = <REPLACE_ELT_max> - (INTVAL (operands[3]) <<
> > <REPLACE_ELT_sh>);
> > +     index = <REPLACE_ELT_V_max>
> > +	     - (INTVAL (operands[3]) << <REPLACE_ELT_V_sh>);
> >  
> >     emit_insn (gen_vreplace_elt_<mode>_inst (operands[0],
> > operands[1],
> >  					    operands[2],
> > @@ -4207,19 +4215,19 @@ (define_expand "vreplace_elt_<mode>"
> >  [(set_attr "type" "vecsimple")])
> >  
> >  (define_insn "vreplace_elt_<mode>_inst"
> > - [(set (match_operand:REPLACE_ELT 0 "register_operand" "=v")
> > -  (unspec:REPLACE_ELT [(match_operand:REPLACE_ELT 1
> > "register_operand" "0")
> > -		       (match_operand:<VEC_base> 2 "register_operand"
> > "r")
> > -		       (match_operand:QI 3 "const_0_to_12_operand"
> > "n")]
> > -		      UNSPEC_REPLACE_ELT))]
> > + [(set (match_operand:REPLACE_ELT_V 0 "register_operand" "=v")
> > +  (unspec:REPLACE_ELT_V [(match_operand:REPLACE_ELT_V 1
> > "register_operand" "0")
> > +			 (match_operand:<VEC_base> 2 "register_operand"
> > "r")
> > +			 (match_operand:QI 3 "const_0_to_12_operand"
> > "n")]
> > +			UNSPEC_REPLACE_ELT))]
> >   "TARGET_POWER10"
> > - "vins<REPLACE_ELT_char> %0,%2,%3"
> > + "vins<REPLACE_ELT_V_char> %0,%2,%3"
> >   [(set_attr "type" "vecsimple")])
> >  
> >  (define_insn "vreplace_un_<mode>"
> >   [(set (match_operand:V16QI 0 "register_operand" "=v")
> > -  (unspec:V16QI [(match_operand:REPLACE_ELT 1 "register_operand"
> > "0")
> > -                 (match_operand:<VEC_base> 2 "register_operand"
> > "r")
> > +  (unspec:V16QI [(match_operand:V16QI 1 "register_operand" "0")
> > +		 (match_operand:REPLACE_ELT 2 "register_operand" "r")
> >  		 (match_operand:QI 3 "const_0_to_12_operand" "n")]
> >  		UNSPEC_REPLACE_UN))]
> >   "TARGET_POWER10"
> > diff --git a/gcc/testsuite/gcc.target/powerpc/vec-replace-word-
> > runnable.c b/gcc/testsuite/gcc.target/powerpc/vec-replace-word-
> > runnable_1.c
> > similarity index 51%
> > rename from gcc/testsuite/gcc.target/powerpc/vec-replace-word-
> > runnable.c
> > rename to gcc/testsuite/gcc.target/powerpc/vec-replace-word-
> > runnable_1.c
> > index 27318822871..7e7485ca371 100644
> > --- a/gcc/testsuite/gcc.target/powerpc/vec-replace-word-runnable.c
> > +++ b/gcc/testsuite/gcc.target/powerpc/vec-replace-word-
> > runnable_1.c
> > @@ -1,5 +1,4 @@
> >  /* { dg-do run { target { power10_hw } } } */
> > -/* { dg-do link { target { ! power10_hw } } } */
> 
> As your explanation that this "dg-do link" has existed for a long
> time,
> I'd a check and found it's from r11-4397-g8d8fef197114a9 "[RS6000]
> Link
> power10 testcases", I think it wanted to test assembler and linker
> when
> there are few actual P10 hw (imagining at the early stage of P10
> support).
> 
> Since there are a few test cases like this, I'm inclined to just
> leave it

OK, put it back in.

                       Carl