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

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

Hi Segher,

On 1/31/22 3:32 PM, Segher Boessenkool wrote:
> 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?

No, as explained above, that code needs to stay in the "special" file
that the build machinery understands.  It looks very difficult to
tease that apart, so I've given up on that.  Sorry!

>
>> 	* 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.

That comment is just moved.  There's a target hook from the vectorizer
for Altivec-style unaligned load masking.  The target needs to provide
a built-in function for this.  The tree contains its function decl.
I can change the comment.

>
>> +/* **** 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.

This entire patch is just moving code.  I already separated that out
from everything else.  The only exception is the one I called out at
the beginning, that removing the builtin_mode_to_type[] array should
have been done previously, and it's convenient to just delete it as
part of this patch.

So I can change these other things that you mention, but that isn't
really the point of this patch, so...  I guess please advise. :-)

Thanks,
Bill

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