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

["Kewen.Lin" <linkw@linux.ibm.com>]

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;