Re: [PATCH] rs6000: Refactor altivec_build_resolved_builtin

2021-12-15 Thread Bill Schmidt via Gcc-patches
Hi!

On 12/15/21 12:16 PM, Segher Boessenkool wrote:
>> +  /* Note:  vec_nand also works but opt changes vec_nand's
>> + to vec_nor's anyway.  */
> Maybe there should be a vec_not?  There is one at the RTL level (called
> one_cmpl2).

As I recall, we have an issue open for this already... but nobody's grabbed it 
yet.

Thanks for the review!

(I'll change all those VEC_* things to lower-case.)

Bill



Re: [PATCH] rs6000: Refactor altivec_build_resolved_builtin

2021-12-15 Thread Segher Boessenkool
Hi!

On Wed, Dec 15, 2021 at 08:21:24AM -0600, Bill Schmidt wrote:
> While replacing the built-in machinery, we agreed to defer some necessary
> refactoring of the overload processing.  This patch cleans it up considerably.
> 
> I've put in one FIXME for an additional level of cleanup that should be done
> independently.  The various helper functions (resolve_VEC_*) can be simplified
> if we move the argument processing in altivec_resolve_overloaded_builtin
> earlier.  But this requires making nontrivial changes to those functions that
> will need careful review.  Let's do that in a later patch.

All these names should be lower case.  If you promise to do that in the
aforementioned cleanup (and it happens before GCC 12), then okay for
trunk.  Thanks!

Very minor stuff below.

>   * config/rs6000/rs6000-c.c (resolution): New enum.
>   (resolve_VEC_MUL): New function.
>   (resolve_VEC_CMPNE): Likewise.
>   (resolve_VEC_ADDE_SUBE): Likewise.
>   (resolve_VEC_ADDEC_SUBEC): Likewise.
>   (resolve_VEC_SPLATS): Likewise.
>   (resolve_VEC_EXTRACT): Likewise.
>   (resolve_VEC_INSERT): Likewise.
>   (resolve_VEC_STEP): Likewise.
>   (find_instance): Likewise.
>   (altivec_resolve_overloaded_builtin): Many cleanups:  Call factored-out

No two spaces after colon, no capital after colon.  You probably want
a full stop though?

>   functions.  Move variable declarations closer to uses.  Add commentary.
>   Remove unnecessary levels of braces.  Avoid use of gotos.  Change
>   misleading variable names.  Use switches over if-else-if chains.

> +   /* Note:  vec_nand also works but opt changes vec_nand's
> +  to vec_nor's anyway.  */

Maybe there should be a vec_not?  There is one at the RTL level (called
one_cmpl2).

> + decl= rs6000_builtin_decls[RS6000_OVLD_VEC_NOR];

Missing space btw.

> -   support Altivec's overloaded builtins.  FIXME: This code needs
> -   to be brutally factored.  */

Yay :-)

>/* Return immediately if this isn't an overload.  */
> +  rs6000_gen_builtins fcode
> += (rs6000_gen_builtins) DECL_MD_FUNCTION_CODE (fndecl);
> +
>if (fcode <= RS6000_OVLD_NONE)
>  return NULL_TREE;

The new code should be before this comment?  I don't see how the comment
makes much sense like this.

> +  /* Some overloads require special handling.  */
> +  /* FIXME: Could we simplify the helper functions if we gathered arguments
> + and types into arrays first?  */

A lot of the argument checking can be handled more generically.  If
there are many exceptions to that it will not be useful, but a bit of
it would make sense.

If you do that (and maybe similar things as well) this array question
will swivel.

> -  if (TREE_CODE (arg0_type) != VECTOR_TYPE)
> - goto bad;
> -  if (!lang_hooks.types_compatible_p (arg0_type, arg1_type))
> - goto bad;

Yay, all gotos should go :-)

Thanks again,


Segher


Re: [PATCH] rs6000: Refactor altivec_build_resolved_builtin

2021-12-09 Thread Bill Schmidt via Gcc-patches
I forgot to point out that this patch is dependent on the pending patches
to remove the old builtins code.

Thanks,
Bill

On 12/9/21 12:33 PM, Bill Schmidt via Gcc-patches wrote:
> Hi!
>
> While replacing the built-in machinery, we agreed to defer some necessary
> refactoring of the overload processing.  This patch cleans it up considerably.
>
> I've put in one FIXME for an additional level of cleanup that should be done
> independently.  The various helper functions (resolve_VEC_*) can be simplified
> if we move the argument processing in altivec_resolve_overloaded_builtin
> earlier.  But this requires making nontrivial changes to those functions that
> will need careful review.  Let's do that in a later patch.
>
> Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.  Is this
> okay for trunk?
>
> Thanks!
> Bill
>
>
> 2021-12-09  Bill Schmidt  
>
> gcc/
>   * config/rs6000/rs6000-c.c (resolution): New enum.
>   (resolve_VEC_MUL): New function.
>   (resolve_VEC_CMPNE): Likewise.
>   (resolve_VEC_ADDE_SUBE): Likewise.
>   (resolve_VEC_ADDEC_SUBEC): Likewise.
>   (resolve_VEC_SPLATS): Likewise.
>   (resolve_VEC_EXTRACT): Likewise.
>   (resolve_VEC_INSERT): Likewise.
>   (resolve_VEC_STEP): Likewise.
>   (find_instance): Likewise.
>   (altivec_resolve_overloaded_builtin): Many cleanups:  Call factored-out
>   functions.  Move variable declarations closer to uses.  Add commentary.
>   Remove unnecessary levels of braces.  Avoid use of gotos.  Change
>   misleading variable names.  Use switches over if-else-if chains.
> ---
>  gcc/config/rs6000/rs6000-c.c | 1717 +++---
>  1 file changed, 945 insertions(+), 772 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000-c.c b/gcc/config/rs6000/rs6000-c.c
> index e0ebdeed548..45f485aab44 100644
> --- a/gcc/config/rs6000/rs6000-c.c
> +++ b/gcc/config/rs6000/rs6000-c.c
> @@ -928,710 +928,939 @@ altivec_build_resolved_builtin (tree *args, int n, 
> tree fntype, tree ret_type,
>return fold_convert (ret_type, call);
>  }
>
> -/* Implementation of the resolve_overloaded_builtin target hook, to
> -   support Altivec's overloaded builtins.  FIXME: This code needs
> -   to be brutally factored.  */
> +/* Enumeration of possible results from attempted overload resolution.
> +   This is used by special-case helper functions to tell their caller
> +   whether they succeeded and what still needs to be done.
>
> -tree
> -altivec_resolve_overloaded_builtin (location_t loc, tree fndecl,
> - void *passed_arglist)
> + unresolved = Still needs processing
> +   resolved = Resolved (but may be an error_mark_node)
> +  resolved_bad = An error that needs handling by the caller.  */
> +
> +enum resolution { unresolved, resolved, resolved_bad };
> +
> +/* Resolve an overloaded vec_mul call and return a tree expression for the
> +   resolved call if successful.  NARGS is the number of arguments to the 
> call.
> +   ARGLIST contains the arguments.  RES must be set to indicate the status of
> +   the resolution attempt.  LOC contains statement location information.  */
> +
> +static tree
> +resolve_VEC_MUL (resolution *res, vec *arglist, unsigned nargs,
> +  location_t loc)
>  {
> -  vec *arglist = static_cast *> 
> (passed_arglist);
> -  unsigned int nargs = vec_safe_length (arglist);
> -  enum rs6000_gen_builtins fcode
> -= (enum rs6000_gen_builtins) DECL_MD_FUNCTION_CODE (fndecl);
> -  tree fnargs = TYPE_ARG_TYPES (TREE_TYPE (fndecl));
> -  tree types[MAX_OVLD_ARGS];
> -  tree args[MAX_OVLD_ARGS];
> +  /* vec_mul needs to be special cased because there are no instructions for 
> it
> + for the {un}signed char, {un}signed short, and {un}signed int types.  */
> +  if (nargs != 2)
> +{
> +  error ("builtin %qs only accepts 2 arguments", "vec_mul");
> +  *res = resolved;
> +  return error_mark_node;
> +}
>
> -  /* Return immediately if this isn't an overload.  */
> -  if (fcode <= RS6000_OVLD_NONE)
> -return NULL_TREE;
> +  tree arg0 = (*arglist)[0];
> +  tree arg0_type = TREE_TYPE (arg0);
> +  tree arg1 = (*arglist)[1];
> +  tree arg1_type = TREE_TYPE (arg1);
>
> -  unsigned int adj_fcode = fcode - RS6000_OVLD_NONE;
> +  /* Both arguments must be vectors and the types must be compatible.  */
> +  if (TREE_CODE (arg0_type) != VECTOR_TYPE
> +  || !lang_hooks.types_compatible_p (arg0_type, arg1_type))
> +{
> +  *res = resolved_bad;
> +  return error_mark_node;
> +}
>
> -  if (TARGET_DEBUG_BUILTIN)
> -fprintf (stderr, "altivec_resolve_overloaded_builtin, code = %4d, %s\n",
> -  (int) fcode, IDENTIFIER_POINTER (DECL_NAME (fndecl)));
> +  switch (TYPE_MODE (TREE_TYPE (arg0_type)))
> +{
> +case E_QImode:
> +case E_HImode:
> +case E_SImode:
> +case E_DImode:
> +case E_TImode:
> +  /* For scalar types just use a multiply expression.  */
> +  *res =