Re: [PATCH v2 1/8] rs6000: More factoring of overload processing
Hi! On 2/1/22 3:48 PM, Segher Boessenkool wrote: > On Tue, Feb 01, 2022 at 08:49:34AM -0600, Bill Schmidt wrote: >> I've modified the previous patch to add more explanatory commentary about >> the number-of-arguments test that was previously confusing, and to convert >> the switch into an if-then-else chain. The rest of the patch is unchanged. >> Bootstrapped and tested on powerpc64le-linux-gnu. Is this okay for trunk? >> gcc/ >> * config/rs6000/rs6000-c.cc (resolve_vec_mul): Accept args and types >> parameters instead of arglist and nargs. Simplify accordingly. Remove >> unnecessary test for argument count mismatch. >> (resolve_vec_cmpne): Likewise. >> (resolve_vec_adde_sube): Likewise. >> (resolve_vec_addec_subec): Likewise. >> (altivec_resolve_overloaded_builtin): Move overload special handling >> after the gathering of arguments into args[] and types[] and the test >> for correct number of arguments. Don't perform the test for correct >> number of arguments for certain special cases. Call the other special >> cases with args and types instead of arglist and nargs. >> + if (fcode != RS6000_OVLD_VEC_PROMOTE >> + && fcode != RS6000_OVLD_VEC_SPLATS >> + && fcode != RS6000_OVLD_VEC_EXTRACT >> + && fcode != RS6000_OVLD_VEC_INSERT >> + && fcode != RS6000_OVLD_VEC_STEP >> + && (!VOID_TYPE_P (TREE_VALUE (fnargs)) || n < nargs)) >> return NULL; > Please don't do De Morgan manually, let the compiler deal with it? > Although even with that the logic is as clear as mud. This matters if > someone (maybe even you) will have to debug this later, or modify this. > Maybe adding some suitably named variables can clarify things here? I can de-deMorgan this. Do you want to see the patch again, or is it okay with that change? Thanks! Bill > >> + if (fcode == RS6000_OVLD_VEC_MUL) >> +returned_expr = resolve_vec_mul (, args, types, loc); >> + else if (fcode == RS6000_OVLD_VEC_CMPNE) >> +returned_expr = resolve_vec_cmpne (, args, types, loc); >> + else if (fcode == RS6000_OVLD_VEC_ADDE || fcode == RS6000_OVLD_VEC_SUBE) >> +returned_expr = resolve_vec_adde_sube (, fcode, args, types, loc); >> + else if (fcode == RS6000_OVLD_VEC_ADDEC || fcode == RS6000_OVLD_VEC_SUBEC) >> +returned_expr = resolve_vec_addec_subec (, fcode, args, types, loc); >> + else if (fcode == RS6000_OVLD_VEC_SPLATS || fcode == >> RS6000_OVLD_VEC_PROMOTE) >> +returned_expr = resolve_vec_splats (, fcode, arglist, nargs); >> + else if (fcode == RS6000_OVLD_VEC_EXTRACT) >> +returned_expr = resolve_vec_extract (, arglist, nargs, loc); >> + else if (fcode == RS6000_OVLD_VEC_INSERT) >> +returned_expr = resolve_vec_insert (, arglist, nargs, loc); >> + else if (fcode == RS6000_OVLD_VEC_STEP) >> +returned_expr = resolve_vec_step (, arglist, nargs); >> + >> + if (res == resolved) >> +return returned_expr; > This is so convoluted because the functions do two things, and have two > return values (res and returned_expr). > > > Segher
Re: [PATCH v2 1/8] rs6000: More factoring of overload processing
On Tue, Feb 01, 2022 at 08:49:34AM -0600, Bill Schmidt wrote: > I've modified the previous patch to add more explanatory commentary about > the number-of-arguments test that was previously confusing, and to convert > the switch into an if-then-else chain. The rest of the patch is unchanged. > Bootstrapped and tested on powerpc64le-linux-gnu. Is this okay for trunk? > gcc/ > * config/rs6000/rs6000-c.cc (resolve_vec_mul): Accept args and types > parameters instead of arglist and nargs. Simplify accordingly. Remove > unnecessary test for argument count mismatch. > (resolve_vec_cmpne): Likewise. > (resolve_vec_adde_sube): Likewise. > (resolve_vec_addec_subec): Likewise. > (altivec_resolve_overloaded_builtin): Move overload special handling > after the gathering of arguments into args[] and types[] and the test > for correct number of arguments. Don't perform the test for correct > number of arguments for certain special cases. Call the other special > cases with args and types instead of arglist and nargs. > + if (fcode != RS6000_OVLD_VEC_PROMOTE > + && fcode != RS6000_OVLD_VEC_SPLATS > + && fcode != RS6000_OVLD_VEC_EXTRACT > + && fcode != RS6000_OVLD_VEC_INSERT > + && fcode != RS6000_OVLD_VEC_STEP > + && (!VOID_TYPE_P (TREE_VALUE (fnargs)) || n < nargs)) > return NULL; Please don't do De Morgan manually, let the compiler deal with it? Although even with that the logic is as clear as mud. This matters if someone (maybe even you) will have to debug this later, or modify this. Maybe adding some suitably named variables can clarify things here? > + if (fcode == RS6000_OVLD_VEC_MUL) > +returned_expr = resolve_vec_mul (, args, types, loc); > + else if (fcode == RS6000_OVLD_VEC_CMPNE) > +returned_expr = resolve_vec_cmpne (, args, types, loc); > + else if (fcode == RS6000_OVLD_VEC_ADDE || fcode == RS6000_OVLD_VEC_SUBE) > +returned_expr = resolve_vec_adde_sube (, fcode, args, types, loc); > + else if (fcode == RS6000_OVLD_VEC_ADDEC || fcode == RS6000_OVLD_VEC_SUBEC) > +returned_expr = resolve_vec_addec_subec (, fcode, args, types, loc); > + else if (fcode == RS6000_OVLD_VEC_SPLATS || fcode == > RS6000_OVLD_VEC_PROMOTE) > +returned_expr = resolve_vec_splats (, fcode, arglist, nargs); > + else if (fcode == RS6000_OVLD_VEC_EXTRACT) > +returned_expr = resolve_vec_extract (, arglist, nargs, loc); > + else if (fcode == RS6000_OVLD_VEC_INSERT) > +returned_expr = resolve_vec_insert (, arglist, nargs, loc); > + else if (fcode == RS6000_OVLD_VEC_STEP) > +returned_expr = resolve_vec_step (, arglist, nargs); > + > + if (res == resolved) > +return returned_expr; This is so convoluted because the functions do two things, and have two return values (res and returned_expr). Segher
[PATCH v2 1/8] rs6000: More factoring of overload processing
Hi,
I've modified the previous patch to add more explanatory commentary about
the number-of-arguments test that was previously confusing, and to convert
the switch into an if-then-else chain. The rest of the patch is unchanged.
Bootstrapped and tested on powerpc64le-linux-gnu. Is this okay for trunk?
Remainder of commit message follows:
This patch continues the refactoring started with r12-6014. I had previously
noted that the resolve_vec* routines can be further simplified by processing
the argument list earlier, so that all routines can use the arrays of arguments
and types. I found that this was useful for some of the routines, but not for
all of them.
For several of the special-cased overloads, we don't specify all of the
possible type combinations in rs6000-overload.def, because the types don't
matter for the expansion we do. For these, we can't use generic error message
handling when the number of arguments is incorrect, because the result is
misleading error messages that indicate argument types are wrong.
So this patch goes halfway and improves the factoring on the remaining special
cases, but leaves vec_splats, vec_promote, vec_extract, vec_insert, and
vec_step alone.
Thanks!
Bill
2022-01-31 Bill Schmidt
gcc/
* config/rs6000/rs6000-c.cc (resolve_vec_mul): Accept args and types
parameters instead of arglist and nargs. Simplify accordingly. Remove
unnecessary test for argument count mismatch.
(resolve_vec_cmpne): Likewise.
(resolve_vec_adde_sube): Likewise.
(resolve_vec_addec_subec): Likewise.
(altivec_resolve_overloaded_builtin): Move overload special handling
after the gathering of arguments into args[] and types[] and the test
for correct number of arguments. Don't perform the test for correct
number of arguments for certain special cases. Call the other special
cases with args and types instead of arglist and nargs.
---
gcc/config/rs6000/rs6000-c.cc | 297 ++
1 file changed, 120 insertions(+), 177 deletions(-)
diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
index 145421ab8f2..4911e5f509c 100644
--- a/gcc/config/rs6000/rs6000-c.cc
+++ b/gcc/config/rs6000/rs6000-c.cc
@@ -939,37 +939,25 @@ altivec_build_resolved_builtin (tree *args, int n, tree
fntype, tree ret_type,
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
+ resolved call if successful. ARGS contains the arguments to the call.
+ TYPES contains their types. 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)
+resolve_vec_mul (resolution *res, tree *args, tree *types, location_t loc)
{
/* 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;
-}
-
- tree arg0 = (*arglist)[0];
- tree arg0_type = TREE_TYPE (arg0);
- tree arg1 = (*arglist)[1];
- tree arg1_type = TREE_TYPE (arg1);
/* 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))
+ if (TREE_CODE (types[0]) != VECTOR_TYPE
+ || !lang_hooks.types_compatible_p (types[0], types[1]))
{
*res = resolved_bad;
return error_mark_node;
}
- switch (TYPE_MODE (TREE_TYPE (arg0_type)))
+ switch (TYPE_MODE (TREE_TYPE (types[0])))
{
case E_QImode:
case E_HImode:
@@ -978,21 +966,21 @@ resolve_vec_mul (resolution *res, vec
*arglist, unsigned nargs,
case E_TImode:
/* For scalar types just use a multiply expression. */
*res = resolved;
- return fold_build2_loc (loc, MULT_EXPR, TREE_TYPE (arg0), arg0,
- fold_convert (TREE_TYPE (arg0), arg1));
+ return fold_build2_loc (loc, MULT_EXPR, types[0], args[0],
+ fold_convert (types[0], args[1]));
case E_SFmode:
{
/* For floats use the xvmulsp instruction directly. */
*res = resolved;
tree call = rs6000_builtin_decls[RS6000_BIF_XVMULSP];
- return build_call_expr (call, 2, arg0, arg1);
+ return build_call_expr (call, 2, args[0], args[1]);
}
case E_DFmode:
{
/* For doubles use the xvmuldp instruction directly. */
*res = resolved;
tree call =
