[1/2] rs6000, add argument to function find_instance

  GCC maintainers:

The rs6000 function find_instance assumes that it is called for built-
ins with only two arguments.  There is no checking for the actual
number of aruguments used in the built-in.  This patch adds an
additional parameter to the function call containing the number of
aruguments in the built-in.  The function will now do the needed checks
for all of the arguments.

This fix is needed for the next patch in the series that fixes the
vec_replace_unaligned built-in.c test.

Please let me know if this patch is acceptable for mainline.  Thanks.

                        Carl 


--------------------------------------------
rs6000, add argument to function find_instance

The function find_instance assumes it is called to check a built-in  with
only two arguments.  Ths patch extends the function by adding a parameter
specifying the number of buit-in arguments to check.

gcc/ChangeLog:
	* config/rs6000/rs6000-c.cc (find_instance): Add new parameter that
	specifies the number of built-in arguments to check.
	(altivec_resolve_overloaded_builtin): Update calls to find_instance
	to pass the number of built-in argument to be checked.
---
 gcc/config/rs6000/rs6000-c.cc | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)
  

Hi Carl,

on 2023/7/18 03:19, Carl Love wrote:
> 
> GCC maintainers:
> 
> The rs6000 function find_instance assumes that it is called for built-
> ins with only two arguments.  There is no checking for the actual
> number of aruguments used in the built-in.  This patch adds an
> additional parameter to the function call containing the number of
> aruguments in the built-in.  The function will now do the needed checks
> for all of the arguments.
> 
> This fix is needed for the next patch in the series that fixes the
> vec_replace_unaligned built-in.c test.
> 
> Please let me know if this patch is acceptable for mainline.  Thanks.
> 
>                         Carl 
> 
> 
> --------------------------------------------
> rs6000, add argument to function find_instance
> 
> The function find_instance assumes it is called to check a built-in  with                                                                     ~~ two spaces.
> only two arguments.  Ths patch extends the function by adding a parameter
                       s/Ths/This/
> specifying the number of buit-in arguments to check.
                          s/bult-in/built-in/

> 
> gcc/ChangeLog:
> 	* config/rs6000/rs6000-c.cc (find_instance): Add new parameter that
> 	specifies the number of built-in arguments to check.
> 	(altivec_resolve_overloaded_builtin): Update calls to find_instance
> 	to pass the number of built-in argument to be checked.

s/argument/arguments/

> ---
>  gcc/config/rs6000/rs6000-c.cc | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
> index a353bca19ef..350987b851b 100644
> --- a/gcc/config/rs6000/rs6000-c.cc
> +++ b/gcc/config/rs6000/rs6000-c.cc
> @@ -1679,7 +1679,7 @@ tree

There is one function comment here describing the meaning of each parameter,
I think we should add a corresponding for NARGS, may be something like:

"; and NARGS specifies the number of built-in arguments."

Also we need to update the below "two"s with "NARGS".

"TYPES contains an array of two types..." and "ARGS contains an array of two arguments..."

since we already extend this to handle NARGS instead of two.

>  find_instance (bool *unsupported_builtin, ovlddata **instance,
>  	       rs6000_gen_builtins instance_code,
>  	       rs6000_gen_builtins fcode,
> -	       tree *types, tree *args)
> +	       tree *types, tree *args, int nargs)
>  {
>    while (*instance && (*instance)->bifid != instance_code)
>      *instance = (*instance)->next;
> @@ -1691,17 +1691,28 @@ find_instance (bool *unsupported_builtin, ovlddata **instance,
>    if (!inst->fntype)
>      return error_mark_node;
>    tree fntype = rs6000_builtin_info[inst->bifid].fntype;
> -  tree parmtype0 = TREE_VALUE (TYPE_ARG_TYPES (fntype));
> -  tree parmtype1 = TREE_VALUE (TREE_CHAIN (TYPE_ARG_TYPES (fntype)));
> +  tree argtype = TYPE_ARG_TYPES (fntype);
> +  tree parmtype;

Nit: We can move "tree parmtype" into the loop (close to its only use).

> +  int args_compatible = true;

s/int/bool/

>  
> -  if (rs6000_builtin_type_compatible (types[0], parmtype0)
> -      && rs6000_builtin_type_compatible (types[1], parmtype1))
> +  for (int i = 0; i <nargs; i++)

Nit: formatting issue, space before nargs.

>      {
> +      parmtype = TREE_VALUE (argtype);

         tree parmtype = TREE_VALUE (argtype);

> +      if (! rs6000_builtin_type_compatible (types[i], parmtype))

Nit: One unexpected(?) space after "!".

> +	{
> +	  args_compatible = false;
> +	  break;
> +	}
> +      argtype = TREE_CHAIN (argtype);
> +    }
> +
> +  if (args_compatible)
> +  {

Nit: indent issue for "{".

Ok for trunk with these nits fixed.  Btw, the description doesn't say
how this was tested, I'm not sure if it's only tested together with
"patch 2/2", but please ensure it's bootstrapped and regress-tested
on BE and LE when committing.  Thanks!

BR,
Kewen

>        if (rs6000_builtin_decl (inst->bifid, false) != error_mark_node
>  	  && rs6000_builtin_is_supported (inst->bifid))
>  	{
>  	  tree ret_type = TREE_TYPE (inst->fntype);
> -	  return altivec_build_resolved_builtin (args, 2, fntype, ret_type,
> +	  return altivec_build_resolved_builtin (args, nargs, fntype, ret_type,
>  						 inst->bifid, fcode);
>  	}
>        else
> @@ -1921,7 +1932,7 @@ altivec_resolve_overloaded_builtin (location_t loc, tree fndecl,
>  	  instance_code = RS6000_BIF_CMPB_32;
>  
>  	tree call = find_instance (&unsupported_builtin, &instance,
> -				   instance_code, fcode, types, args);
> +				   instance_code, fcode, types, args, nargs);
>  	if (call != error_mark_node)
>  	  return call;
>  	break;
> @@ -1958,7 +1969,7 @@ altivec_resolve_overloaded_builtin (location_t loc, tree fndecl,
>  	  }
>  
>  	tree call = find_instance (&unsupported_builtin, &instance,
> -				   instance_code, fcode, types, args);
> +				   instance_code, fcode, types, args, nargs);
>  	if (call != error_mark_node)
>  	  return call;
>  	break;
  

On Fri, 2023-07-21 at 10:19 +0800, Kewen.Lin wrote:
> Hi Carl,
> 
> on 2023/7/18 03:19, Carl Love wrote:
> > GCC maintainers:
> > 
> > The rs6000 function find_instance assumes that it is called for
> > built-
> > ins with only two arguments.  There is no checking for the actual
> > number of aruguments used in the built-in.  This patch adds an
> > additional parameter to the function call containing the number of
> > aruguments in the built-in.  The function will now do the needed
> > checks
> > for all of the arguments.
> > 
> > This fix is needed for the next patch in the series that fixes the
> > vec_replace_unaligned built-in.c test.
> > 
> > Please let me know if this patch is acceptable for
> > mainline.  Thanks.
> > 
> >                         Carl 
> > 
> > 
> > --------------------------------------------
> > rs6000, add argument to function find_instance
> > 
> > The function find_instance assumes it is called to check a built-
> > in  with                               

Fixed
> >                                       ~~ two spaces.
> > only two arguments.  Ths patch extends the function by adding a
> > parameter
>                        s/Ths/This/
> > specifying the number of buit-in arguments to check.
>                           s/bult-in/built-in/
> 
Fixed both typos.

> > gcc/ChangeLog:
> > 	* config/rs6000/rs6000-c.cc (find_instance): Add new parameter
> > that
> > 	specifies the number of built-in arguments to check.
> > 	(altivec_resolve_overloaded_builtin): Update calls to
> > find_instance
> > 	to pass the number of built-in argument to be checked.
> 
> s/argument/arguments/
fixed
> 
> > ---
> >  gcc/config/rs6000/rs6000-c.cc | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> > 
> > diff --git a/gcc/config/rs6000/rs6000-c.cc
> > b/gcc/config/rs6000/rs6000-c.cc
> > index a353bca19ef..350987b851b 100644
> > --- a/gcc/config/rs6000/rs6000-c.cc
> > +++ b/gcc/config/rs6000/rs6000-c.cc
> > @@ -1679,7 +1679,7 @@ tree
> 
> There is one function comment here describing the meaning of each
> parameter,
> I think we should add a corresponding for NARGS, may be something
> like:
> 
> "; and NARGS specifies the number of built-in arguments."
> 
Added NARGS description.

> Also we need to update the below "two"s with "NARGS".
> 
> "TYPES contains an array of two types..." and "ARGS contains an array
> of two arguments..."
> 

Replaced multiple "two" occurrences with NARGS.

> since we already extend this to handle NARGS instead of two.
> 
> >  find_instance (bool *unsupported_builtin, ovlddata **instance,
> >  	       rs6000_gen_builtins instance_code,
> >  	       rs6000_gen_builtins fcode,
> > -	       tree *types, tree *args)
> > +	       tree *types, tree *args, int nargs)
> >  {
> >    while (*instance && (*instance)->bifid != instance_code)
> >      *instance = (*instance)->next;
> > @@ -1691,17 +1691,28 @@ find_instance (bool *unsupported_builtin,
> > ovlddata **instance,
> >    if (!inst->fntype)
> >      return error_mark_node;
> >    tree fntype = rs6000_builtin_info[inst->bifid].fntype;
> > -  tree parmtype0 = TREE_VALUE (TYPE_ARG_TYPES (fntype));
> > -  tree parmtype1 = TREE_VALUE (TREE_CHAIN (TYPE_ARG_TYPES
> > (fntype)));
> > +  tree argtype = TYPE_ARG_TYPES (fntype);
> > +  tree parmtype;
> 
> Nit: We can move "tree parmtype" into the loop (close to its only
> use).

Moved and combined declaration with assignment as you noted below.

> 
> > +  int args_compatible = true;
> 
> s/int/bool/
Changed.

> 
> >  
> > -  if (rs6000_builtin_type_compatible (types[0], parmtype0)
> > -      && rs6000_builtin_type_compatible (types[1], parmtype1))
> > +  for (int i = 0; i <nargs; i++)
> 
> Nit: formatting issue, space before nargs.
> 
> >      {
> > +      parmtype = TREE_VALUE (argtype);
> 
>          tree parmtype = TREE_VALUE (argtype);

Changed

> 
> > +      if (! rs6000_builtin_type_compatible (types[i], parmtype))
> 
> Nit: One unexpected(?) space after "!".

Removed extra space after "!".
> 
> > +	{
> > +	  args_compatible = false;
> > +	  break;
> > +	}
> > +      argtype = TREE_CHAIN (argtype);
> > +    }
> > +
> > +  if (args_compatible)
> > +  {
> 
> Nit: indent issue for "{".
Fixed indent.

> 
> Ok for trunk with these nits fixed.  Btw, the description doesn't say
> how this was tested, I'm not sure if it's only tested together with
> "patch 2/2", but please ensure it's bootstrapped and regress-tested
> on BE and LE when committing.  Thanks!
> 

Yes, it was tested with patch 2/2 on Power 10 LE.  I did do a test on
Power 9 as well but don't recall if I tested for both BE and LE.  Will
retest on Power 8 LE/BE, Power 9 LE/BE and Power 10.

                 Carl