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
