[PATCH 14/18] rs6000: Debug support

[Segher Boessenkool]

On Wed, Sep 01, 2021 at 11:13:50AM -0500, Bill Schmidt wrote:
> 	* config/rs6000/rs6000-call.c (rs6000_debug_type): New function.
> 	(def_builtin): Change debug formatting for easier parsing and
> 	include more information.
> 	(rs6000_init_builtins): Add dump of autogenerated builtins.
> 	(altivec_init_builtins): Dump __builtin_altivec_mask_for_load for
> 	completeness.

>  /* Builtins.  */

Maybe change this header now?  It heads "def_builtin" before this patch,
maybe it should just move back there?  And as any function comment, it
should describe the function arguments (and return value, but that is
void there :-) )

> +/* Debug utility to translate a type node to a single token.  */

It returns a text string, instead.  The function name can be better:

> +static
> +const char *rs6000_debug_type (tree type)

"debug_" suggests this function outputs something to stderr.

> +{
> +  if (type == void_type_node)
> +    return "void";
> +  else if (type == long_integer_type_node)
> +    return "long";
> +  else if (type == long_unsigned_type_node)
> +    return "ulong";
> +  else if (type == long_long_integer_type_node)
> +    return "longlong";
> +  else if (type == long_long_unsigned_type_node)
> +    return "ulonglong";
> +  else if (type == bool_V2DI_type_node)
> +    return "vbll";
> +  else if (type == bool_V4SI_type_node)
> +    return "vbi";
> +  else if (type == bool_V8HI_type_node)
> +    return "vbs";
> +  else if (type == bool_V16QI_type_node)
> +    return "vbc";
> +  else if (type == bool_int_type_node)
> +    return "bool";
> +  else if (type == dfloat64_type_node)
> +    return "_Decimal64";
> +  else if (type == double_type_node)
> +    return "double";
> +  else if (type == intDI_type_node)
> +    return "sll";
> +  else if (type == intHI_type_node)
> +    return "ss";
> +  else if (type == ibm128_float_type_node)
> +    return "__ibm128";
> +  else if (type == opaque_V4SI_type_node)
> +    return "opaque";
> +  else if (POINTER_TYPE_P (type))
> +    return "void*";
> +  else if (type == intQI_type_node || type == char_type_node)
> +    return "sc";
> +  else if (type == dfloat32_type_node)
> +    return "_Decimal32";
> +  else if (type == float_type_node)
> +    return "float";
> +  else if (type == intSI_type_node || type == integer_type_node)
> +    return "si";
> +  else if (type == dfloat128_type_node)
> +    return "_Decimal128";
> +  else if (type == long_double_type_node)
> +    return "longdouble";
> +  else if (type == intTI_type_node)
> +    return "sq";
> +  else if (type == unsigned_intDI_type_node)
> +    return "ull";
> +  else if (type == unsigned_intHI_type_node)
> +    return "us";
> +  else if (type == unsigned_intQI_type_node)
> +    return "uc";
> +  else if (type == unsigned_intSI_type_node)
> +    return "ui";
> +  else if (type == unsigned_intTI_type_node)
> +    return "uq";
> +  else if (type == unsigned_V1TI_type_node)
> +    return "vuq";
> +  else if (type == unsigned_V2DI_type_node)
> +    return "vull";
> +  else if (type == unsigned_V4SI_type_node)
> +    return "vui";
> +  else if (type == unsigned_V8HI_type_node)
> +    return "vus";
> +  else if (type == unsigned_V16QI_type_node)
> +    return "vuc";
> +  else if (type == V16QI_type_node)
> +    return "vsc";
> +  else if (type == V1TI_type_node)
> +    return "vsq";
> +  else if (type == V2DF_type_node)
> +    return "vd";
> +  else if (type == V2DI_type_node)
> +    return "vsll";
> +  else if (type == V4SF_type_node)
> +    return "vf";
> +  else if (type == V4SI_type_node)
> +    return "vsi";
> +  else if (type == V8HI_type_node)
> +    return "vss";
> +  else if (type == pixel_V8HI_type_node)
> +    return "vp";
> +  else if (type == pcvoid_type_node)
> +    return "voidc*";
> +  else if (type == float128_type_node)
> +    return "_Float128";
> +  else if (type == vector_pair_type_node)
> +    return "__vector_pair";
> +  else if (type == vector_quad_type_node)
> +    return "__vector_quad";
> +  else
> +    return "unknown";
> +}

Please use a switch statement for this.  You can call the variable
"type_node" then as well, which would be a good idea.

>    if (TARGET_DEBUG_BUILTIN)
> -    fprintf (stderr, "rs6000_builtin, code = %4d, %s%s\n",
> -	     (int)code, name, attr_string);
> +    {
> +      tree t = TREE_TYPE (type);
> +      fprintf (stderr, "%s %s (", rs6000_debug_type (t), name);
> +      t = TYPE_ARG_TYPES (type);
> +      while (t && TREE_VALUE (t) != void_type_node)

Can both 0 and void_type_node terminate a list here?

> +	{
> +	  fprintf (stderr, "%s",
> +		   rs6000_debug_type (TREE_VALUE (t)));

This easily fits on one line.

> +	  t = TREE_CHAIN (t);
> +	  if (t && TREE_VALUE (t) != void_type_node)
> +	    fprintf (stderr, ", ");

It is easier to use a "bool first" extra var, you do not need to write
the same condition twice that way.

  bool first = true;
  while (...)
    {
      if (!first)
	fprintf ...;
      first = false;

      rest of loop body;
    }


> +	}
> +      fprintf (stderr, "); %s [%4d]\n", attr_string, (int)code);
> +    }

(space after cast)

>  
>  static const struct builtin_compatibility bdesc_compat[] =
> @@ -16097,6 +16209,67 @@ rs6000_init_builtins (void)
>    /* Execute the autogenerated initialization code for builtins.  */
>    rs6000_init_generated_builtins ();
>  
> +  if (TARGET_DEBUG_BUILTIN)
> +     {

(misindent)

> +	  if (e == ENB_P10_64 && (!TARGET_POWER10 || !TARGET_POWERPC64))

	  if (e == ENB_P10_64 && !(TARGET_POWER10 && TARGET_POWERPC64))

It even is shorter in this case ;-)

> +  if (TARGET_DEBUG_BUILTIN)
> +    fprintf (stderr, "%s __builtin_altivec_mask_for_load (%s); [%4d]\n",
> +	     rs6000_debug_type (TREE_TYPE (v16qi_ftype_pcvoid)),
> +	     rs6000_debug_type (TREE_VALUE
> +				(TYPE_ARG_TYPES (v16qi_ftype_pcvoid))),

Never start a line with a paren from a function call.  Often using an
extra variable is the best solution?

Okay for trunk with those things touched up.  Thanks!


Segher