Re: [PATCH 01/18] rs6000: Handle overloads during program parsing

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

On Wed, Sep 01, 2021 at 11:13:37AM -0500, Bill Schmidt wrote:
> Although this patch looks quite large, the changes are fairly minimal.
> Most of it is duplicating the large function that does the overload
> resolution using the automatically generated data structures instead of
> the old hand-generated ones.  This doesn't make the patch terribly easy to
> review, unfortunately.  Just be aware that generally we aren't changing
> the logic and functionality of overload handling.

> 	(altivec_build_new_resolved_builtin): New function.
> 	(altivec_resolve_new_overloaded_builtin): Likewise.

A new function of 973 lines (plus the function comment).  Please factor
that (can be in a later patch, but please do, you know what it all means
and does currently, now is the time :-) ).

> +static bool
> +rs6000_new_builtin_type_compatible (tree t, tree u)

This needs a function comment.  Are t and u used symmetrically at all?

> +{
> +  if (t == error_mark_node)
> +    return false;

(not here)

> +  if (POINTER_TYPE_P (t) && POINTER_TYPE_P (u))
> +    {
> +      t = TREE_TYPE (t);
> +      u = TREE_TYPE (u);
> +      if (TYPE_READONLY (u))
> +	t = build_qualified_type (t, TYPE_QUAL_CONST);
> +    }

Esp. here.  And it still creates junk trees where those are not needed
afaics, and that is not a great idea for functions that are called so
often.

> +static tree
> +altivec_build_new_resolved_builtin (tree *args, int n, tree fntype,
> +				    tree ret_type,
> +				    rs6000_gen_builtins bif_id,
> +				    rs6000_gen_builtins ovld_id)
> +{
> +  tree argtypes = TYPE_ARG_TYPES (fntype);
> +  tree arg_type[MAX_OVLD_ARGS];
> +  tree fndecl = rs6000_builtin_decls_x[bif_id];
> +  tree call;

Don't declare things so far ahead please.  Declare them right before
they are assigned to, ideally.

> +  for (int i = 0; i < n; i++)
> +    arg_type[i] = TREE_VALUE (argtypes), argtypes = TREE_CHAIN (argtypes);

Please do not use comma operators where you could use separate
statements.

> +  /* The AltiVec overloading implementation is overall gross, but this

Ooh you spell "AltiVec" correctly here ;-)

You can do
  for (int j = 0; j < n; j++)
    args[j] = fully_fold_convert (arg_type[j], args[j]);
here and then the rest becomes simpler.

> +  switch (n)
> +    {
> +    case 0:
> +      call = build_call_expr (fndecl, 0);
> +      break;
> +    case 1:
> +      call = build_call_expr (fndecl, 1,
> +			      fully_fold_convert (arg_type[0], args[0]));
> +      break;
> +    case 2:
> +      call = build_call_expr (fndecl, 2,
> +			      fully_fold_convert (arg_type[0], args[0]),
> +			      fully_fold_convert (arg_type[1], args[1]));
> +      break;
> +    case 3:
> +      call = build_call_expr (fndecl, 3,
> +			      fully_fold_convert (arg_type[0], args[0]),
> +			      fully_fold_convert (arg_type[1], args[1]),
> +			      fully_fold_convert (arg_type[2], args[2]));
> +      break;
> +    case 4:
> +      call = build_call_expr (fndecl, 4,
> +			      fully_fold_convert (arg_type[0], args[0]),
> +			      fully_fold_convert (arg_type[1], args[1]),
> +			      fully_fold_convert (arg_type[2], args[2]),
> +			      fully_fold_convert (arg_type[3], args[3]));
> +      break;
> +    default:
> +      gcc_unreachable ();
> +    }
> +  return fold_convert (ret_type, call);
> +}

> +static tree
> +altivec_resolve_new_overloaded_builtin (location_t loc, tree fndecl,
> +					void *passed_arglist)
> +{
> +  vec<tree, va_gc> *arglist = static_cast<vec<tree, va_gc> *> (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], args[MAX_OVLD_ARGS];

Two separate lines please, they are very different things, and very
important things, too.

> +  unsigned int n;

You use this var first 792 lines later.  Please don't.

Oh well, this will become much better once this is more properly
factored.  Who knows, some of it may become readable / understandable
even!  :-)

> +      arg = (*arglist)[0];
> +      type = TREE_TYPE (arg);
> +      if (!SCALAR_FLOAT_TYPE_P (type)
> +	  && !INTEGRAL_TYPE_P (type))
> +	goto bad;

And all gotos still scream "FACTOR ME".

> +	  case E_TImode:
> +	    type = (unsigned_p ? unsigned_V1TI_type_node : V1TI_type_node);
> +	    size = 1;
> +	    break;

  type = signed_or_unsigned_type_for (unsigned_p, V1TI_type_node);
etc.

> +	arg2 = build_binary_op (loc, BIT_AND_EXPR, arg2,
> +				build_int_cst (TREE_TYPE (arg2),
> +					       TYPE_VECTOR_SUBPARTS (arg1_type)
> +					       - 1), 0);

This needs some temporaries.  Whenever you are clutching the right
margin chances are you should add some extra names for readability.

> +	  if (TYPE_READONLY (TREE_TYPE (type))
> +	      && !TYPE_READONLY (TREE_TYPE (decl_type)))
> +	    warning (0, "passing argument %d of %qE discards qualifiers from "
> +		     "pointer target type", n + 1, fndecl);

It actually only tests the const qualifier.  Is there no utility
function to test all (or at least cv)?

> +	  type = build_pointer_type (build_qualified_type (TREE_TYPE (type),
> +							   0));

No new line needed.

> +	if ((GET_MODE_PRECISION (arg1_mode) > 32)
> +	    || (GET_MODE_PRECISION (arg2_mode) > 32))

Useless extra parens making things harder to read.

> +bool
> +rs6000_new_builtin_is_supported (enum rs6000_gen_builtins fncode)
> +{
> +  switch (rs6000_builtin_info_x[(size_t) fncode].enable)
> +    {
> +    default:
> +      gcc_unreachable ();

default belongs last, not first.

> +    case ENB_ALWAYS:
> +      return true;
> +    case ENB_P5:
> +      return TARGET_POPCNTB;
> +    case ENB_P6:
> +      return TARGET_CMPB;
> +    case ENB_ALTIVEC:
> +      return TARGET_ALTIVEC;
> +    case ENB_CELL:
> +      return TARGET_ALTIVEC && rs6000_cpu == PROCESSOR_CELL;
> +    case ENB_VSX:
> +      return TARGET_VSX;
> +    case ENB_P7:
> +      return TARGET_POPCNTD;
> +    case ENB_P7_64:
> +      return TARGET_POPCNTD && TARGET_POWERPC64;
> +    case ENB_P8:
> +      return TARGET_DIRECT_MOVE;
> +    case ENB_P8V:
> +      return TARGET_P8_VECTOR;
> +    case ENB_P9:
> +      return TARGET_MODULO;
> +    case ENB_P9_64:
> +      return TARGET_MODULO && TARGET_POWERPC64;
> +    case ENB_P9V:
> +      return TARGET_P9_VECTOR;
> +    case ENB_IEEE128_HW:
> +      return TARGET_FLOAT128_HW;
> +    case ENB_DFP:
> +      return TARGET_DFP;
> +    case ENB_CRYPTO:
> +      return TARGET_CRYPTO;
> +    case ENB_HTM:
> +      return TARGET_HTM;
> +    case ENB_P10:
> +      return TARGET_POWER10;
> +    case ENB_P10_64:
> +      return TARGET_POWER10 && TARGET_POWERPC64;
> +    case ENB_MMA:
> +      return TARGET_MMA;
> +    }
> +  gcc_unreachable ();
> +}

Could you put all the CPU ones together (except maybe Cell)?  The really
mean architecture version, and they should be renamed some day perhaps
(the TARGET_ names that is).  It now is some kind of revisionist
historical order :-)

> --- a/gcc/config/rs6000/rs6000-gen-builtins.c
> +++ b/gcc/config/rs6000/rs6000-gen-builtins.c
> @@ -2314,7 +2314,7 @@ write_decls (void)
>  
>    fprintf (header_file, "extern void rs6000_init_generated_builtins ();\n\n");
>    fprintf (header_file,
> -	   "extern bool rs6000_new_builtin_is_supported_p "
> +	   "extern bool rs6000_new_builtin_is_supported "
>  	   "(rs6000_gen_builtins);\n");

This now fits on one line, too :-)


Okay for trunk with the trivial things fixed.  And everythin else needs
to be fixed later still.

Thanks!


Segher