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

[Bill Schmidt <wschmidt@linux.ibm.com>]

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 (&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