Re: [PATCH v2 1/8] rs6000: More factoring of overload processing

[Segher Boessenkool <segher@kernel.crashing.org>]

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 (&res, args, types, loc);
> +  else if (fcode == RS6000_OVLD_VEC_CMPNE)
> +    returned_expr = resolve_vec_cmpne (&res, args, types, loc);
> +  else if (fcode == RS6000_OVLD_VEC_ADDE || fcode == RS6000_OVLD_VEC_SUBE)
> +    returned_expr = resolve_vec_adde_sube (&res, fcode, args, types, loc);
> +  else if (fcode == RS6000_OVLD_VEC_ADDEC || fcode == RS6000_OVLD_VEC_SUBEC)
> +    returned_expr = resolve_vec_addec_subec (&res, fcode, args, types, loc);
> +  else if (fcode == RS6000_OVLD_VEC_SPLATS || fcode == RS6000_OVLD_VEC_PROMOTE)
> +    returned_expr = resolve_vec_splats (&res, fcode, arglist, nargs);
> +  else if (fcode == RS6000_OVLD_VEC_EXTRACT)
> +    returned_expr = resolve_vec_extract (&res, arglist, nargs, loc);
> +  else if (fcode == RS6000_OVLD_VEC_INSERT)
> +    returned_expr = resolve_vec_insert (&res, arglist, nargs, loc);
> +  else if (fcode == RS6000_OVLD_VEC_STEP)
> +    returned_expr = resolve_vec_step (&res, 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