Re: [RFC PATCH v1 1/1] PPC64: Implement POWER Architecture Vector Function ABI.

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

Hi!

This is about the Power binding to some OpenMP API, right?  It has
nothing to do with "vector" or "ABI" -- we have vectors already, and
we have ABIs already, more than enough of each.

It is very very VERY hard to review this without being told the proper
setting here.


On Fri, Aug 07, 2020 at 08:35:52PM +0000, Bert Tenjy wrote:
> This patch adds functionality to enable use of POWER Architecture's
> VSX extensions to speed up certain code sequences.

It does?  Oh, to implement some OpenMP stuff?

> The document describing POWER Architecture Vector Function interface is
> tentatively at: https://sourceware.org/glibc/wiki/Homepage?action=AttachFile&
> do=view&target=powerarchvectfuncabi.html

"This page does not exist yet. You can create a new empty page, or use
one of the page templates."

> 4. Changes to files vect-simd-clone-{1,4,5,8}.c are needed since 
> PPC64 has only 128bit-wide vector bus. x86_64 for which the tests were
> initially written has buses wider than that for AVX and higher architectures.

There is no "vector bus".  All Power vector registers are 128 bits, yes.

> 5. Per Segher's response to v0, we still need to agree a name for the 
> guiding document whose name is currently 'POWER Architecture Vector Function 
> ABI'.

Not just the document title.  You should use terminology that agrees with
everything else, that isn't usiing the same words for different things,
that isn't super confusing, throughout the patch :-)

> +/* Implement TARGET_SIMD_CLONE_COMPUTE_VECSIZE_AND_SIMDLEN.  */

The documentation for this hook says ((lack of) line wraps verbatim):

@deftypefn {Target Hook} int TARGET_SIMD_CLONE_COMPUTE_VECSIZE_AND_SIMDLEN (struct cgraph_node *@var{}, struct cgraph_simd_clone *@var{}, @var{tree}, @var{int})
This hook should set @var{vecsize_mangle}, @var{vecsize_int}, @var{vecsize_float}
fields in @var{simd_clone} structure pointed by @var{clone_info} argument and also
@var{simdlen} field if it was previously 0.
The hook should return 0 if SIMD clones shouldn't be emitted,
or number of @var{vecsize_mangle} variants that should be emitted.
@end deftypefn

so I have no idea what this hook should do.  Two of the four arguments
are left completely undefined, to start with.

> +
> +static int
> +rs6000_simd_clone_compute_vecsize_and_simdlen (struct cgraph_node *node,
> +                                        struct cgraph_simd_clone *clonei,
> +                                        tree base_type, int num)

Indent is wrong here, btw (should use tabs, and everything should align
to that first "struct"...)  You need to be a bit more creative here,
maybe use shorter function names to begin with?  And use names that say
what the functions are *for*, or giving some context.

"simd" means nothing here.  More than  half of our backend is SIMD
stuff.  That has only sideways to do with what you call "simd" here :-/

> +  tree t;
> +  int i;

Declare things at first use, *in* the first use if you can (you usually
can).

> +  bool decl_arg_p = (node->definition || type_arg_types == NULL_TREE);

This isn't a predicate, don't call it _p please.  It's a boolean, and
"decl_arg" isn't very meaningful either.

You might want to factor this differently altogether.

> +  for (t = (decl_arg_p ? DECL_ARGUMENTS (node->decl) : type_arg_types), i = 0;
> +       t && t != void_list_node; t = TREE_CHAIN (t), i++)

Do all that "i" stuff not in the "for" expression itself (but in the
body).  If a for expression doesn't fit on one line, it usually is more
readable if you put all three parts on separate lines.  Complex
initialisation like here is more readable if you do it *before* the
loop.

> +	case E_QImode:
> +	case E_HImode:
> +	case E_SImode:
> +	case E_DImode:
> +	case E_SFmode:
> +	case E_DFmode:

> +	  warning_at (DECL_SOURCE_LOCATION (node->decl), 0,
> +		      "unsupported argument type %qT for simd", arg_type);

That isn't all types.  But you do ISA 2.07?  Put that in a comment
somewhere then please.

> +  if (TARGET_VSX)
> +    {
> +      clonei->vecsize_mangle = 'b';
> +      ret = 1;
> +    }

That is ISA 2.06 (Power 7), which you do not support I think?

> +  switch (clonei->vecsize_mangle)

I don't know what this is.

> +void
> +rs6000_simd_clone_adjust (struct cgraph_node *node)
> +{
> +}

Don't define it if it doesn't do anything?  Or is it required to exist
even if it doesn't ever do anything?

> +static int
> +rs6000_simd_clone_usable (struct cgraph_node *node)
> +{
> +  switch (node->simdclone->vecsize_mangle)
> +    {
> +      case 'b':

(wrong indentation, "case" should align with "{")

> +        if (!TARGET_VSX)
> +          return -1;
> +        return 0;
> +      default:
> +        gcc_unreachable ();
> +    }
> +}

Please don't use switch statements where a simple "if" would do.

static int
rs6000_simd_clone_usable (struct cgraph_node *node)
{
  gcc_assert (node->simdclone->vecsize_mangle == 'b');

  if (TARGET_VSX)
    return 0;

  return -1;
}

(If it looks too complicated, it probably is; don't remove whitelines to
make it shorter, that only makes things worse).


Anyway, please give some context in the proposed commit message: like
pointing to *what* it implements, and where that is described, and where
the binding is described.  And no dead links please ;-)


Segher