Re: [PATCH 4/8] rs6000: Consolidate target built-ins code

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

Hi!

On Fri, Jan 28, 2022 at 11:50:22AM -0600, Bill Schmidt wrote:
> Continuing with the refactoring effort, this patch moves as much of the
> target-specific built-in support code into a new file, rs6000-builtin.cc.
> However, we can't easily move the overloading support code out of
> rs6000-c.cc, because the build machinery understands that as a special file
> to be included with the C and C++ front ends.

And the other C-like frontends.

> This patch is just a straightforward move, with one exception.  I found
> that the builtin_mode_to_type[] array is no longer used, so I also removed
> all code having to do with it.

Oh nice, your rewrite removed the need for that array.  Great :-)

> The code in rs6000-builtin.cc is organized in related sections:
>  - General support functions
>  - Initialization support
>  - GIMPLE folding support
>  - Expansion support
> 
> Overloading support remains in rs6000-c.cc.

So, what is needed to move that as well?  Is moving that in the plan?

> 	* config/rs6000/rs6000-builtin.cc: New file, containing code moved
> 	from other files.

(You're breaking lines early again.)

> -	extra_objs="${extra_objs} rs6000-builtins.o"
> +	extra_objs="${extra_objs} rs6000-builtins.o rs6000-builtin.o"

It's pretty unfortunate that these files are named alike.  The source
files exist in different places of course, so the danger of confusion
is minimal usually.

> +/* Support targetm.vectorize.builtin_mask_for_load.  */
> +tree altivec_builtin_mask_for_load;

"Support"?  What does that mean?  Please describe what this tree is.

> +/* **** General support functions. **** */

This isn't a sentence so should not have a full stop.  (And otherwise
it should be followed by two spaces!)

> +bool
> +rs6000_builtin_is_supported (enum rs6000_gen_builtins fncode)
> +{
> +  switch (rs6000_builtin_info[(size_t) fncode].enable)
> +    {
> +    case ENB_ALWAYS:
> +      return true;
> +    case ENB_P5:
> +      return TARGET_POPCNTB;
> +    case ENB_P6:
> +      return TARGET_CMPB;
> +    case ENB_P6_64:
> +      return TARGET_CMPB && TARGET_POWERPC64;
> +    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_P10:
> +      return TARGET_POWER10;
> +    case ENB_P10_64:
> +      return TARGET_POWER10 && TARGET_POWERPC64;
> +    case ENB_ALTIVEC:
> +      return TARGET_ALTIVEC;
> +    case ENB_VSX:
> +      return TARGET_VSX;
> +    case ENB_CELL:
> +      return TARGET_ALTIVEC && rs6000_cpu == PROCESSOR_CELL;
> +    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_MMA:
> +      return TARGET_MMA;
> +    default:
> +      gcc_unreachable ();
> +    }
> +  gcc_unreachable ();
> +}

If you rewrite this without switch it is shorter and clearer, and you do
not need to duplicate the gcc_unreachable (which the broken warning
forces you to).

> +  if (fcode >= RS6000_OVLD_MAX)
> +    return error_mark_node;

This shows that that isn't really the max, it is the number of elts in
the array, instead (maximum is inclusive).  Maybe fis that some day :-)

> +/* Implement targetm.vectorize.builtin_md_vectorized_function.  */
> +
> +tree
> +rs6000_builtin_md_vectorized_function (tree fndecl, tree type_out,
> +				       tree type_in)
> +{
> +  machine_mode in_mode, out_mode;
> +  int in_n, out_n;
> +
> +  if (TARGET_DEBUG_BUILTIN)
> +    fprintf (stderr,
> +	     "rs6000_builtin_md_vectorized_function (%s, %s, %s)\n",
> +	     IDENTIFIER_POINTER (DECL_NAME (fndecl)),
> +	     GET_MODE_NAME (TYPE_MODE (type_out)),
> +	     GET_MODE_NAME (TYPE_MODE (type_in)));
> +
> +  /* TODO: Should this be gcc_assert?  */
> +  if (TREE_CODE (type_out) != VECTOR_TYPE
> +      || TREE_CODE (type_in) != VECTOR_TYPE)
> +    return NULL_TREE;

Yes, as target.def says.

> +  enum rs6000_gen_builtins fn
> +    = (enum rs6000_gen_builtins) DECL_MD_FUNCTION_CODE (fndecl);

You could write
       = (decltype (fn)) DECL_MD_FUNCTION_CODE (fndecl);
btw.

> +    default:
> +      break;

Another wart :-(

The -Wswitch* warnings this works around should not be part of -Wall
imo, not even of -Wextra :-(

> +static
> +const char *rs6000_type_string (tree type_node)
> +{
> +  if (type_node == void_type_node)
> +    return "void";
> +  else if (type_node == long_integer_type_node)
> +    return "long";

You do not need the "else" btw.  Getting rid of that will make the code
easier to read.

> +void
> +rs6000_init_builtins (void)
> +{
> +  tree tdecl;
> +  tree t;

Don't declare things long before they are used please.

...

Bah, I am commenting on existing code...  Please do not mix code
movement and other things in the same patch.  In the (very few!) cases
where you need things to stay together for bisectability, you can still
submit it as multiple patches (and you have that, that is how you write
it in the first place!), just comment that you need to check in a few
patches together.

Is it a big hassle to do that now, for this patch?


Segher