Re: [PING]: [GOMP4] [PATCH] SIMD-Enabled Functions (formerly Elemental functions) for C

[Aldy Hernandez <aldyh at redhat dot com>]

On 11/30/13 20:38, Iyer, Balaji V wrote:
> Hello Aldy,
> 	Some of the middle end changes I made in the previous patch was not flying for the C++. Here is a fixed patch where the middle-end changes will work for both C and C++.
> 	With this email, I am attaching the patch for C along with the middle end changes. Is this Ok for the branch?

Jakub and company ultimately have to approve your patch, but here are a 
few things.

> +
> +  /* Cilk Plus specific parser/lexer information.  */
> +
> +  /* Buffer to hold all the tokens from parsing the vector attribute for the
> +     SIMD Enabled functions (formerly known as elemental functions).  */
> +  vec <c_token, va_gc> *elem_fn_tokens;

If the name "elementals" is being phased out, then perhaps you need to 
come up with another name here.  Perhaps "cilk_simd_clone_tokens" or 
something that looks similar to the OpenMP variant 
"cilk_declare_simd_tokens" (akin to omp_declare_simd_clauses) in the 
rest of the patch.

Also, "Enabled" should not be capitalized.

> +/* Parses the vectorlength vector attribute for the SIMD Enabled functions
> +   in Cilk Plus.
> +   Syntax:
> +   vectorlength (<integer constant expression>)  */
> +
> +static bool
> +c_parser_elem_fn_vectorlength (c_parser *parser)

Similarly here.  Let's get rid of *elem* nomenclature throughout. 
Perhaps c_parser_cilk_declare_simd_vectorlength and similarly throughout 
the other parsing routines in the patch.  This will make it clearer that 
*cilk_declare_simd* is related to OpenMP's declare simd.

> +  if (TREE_CODE (value) != INTEGER_CST)
> +    {
> +      error_at (token->location, "vectorlength must be a constant integer");
> +      c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
> +      return false;
> +    }

I thought integer constant expressions were allowed here.  Shouldn't 
things like "sizeof (int)" be allowed?  See what is done in 
c_parser_cilk_clause_vectorlength() which handles constant expressions. 
 Also, you will need a corresponding test.

For that matter... can't we combine the above function with 
c_parser_cilk_clause_vectorlength().  It doesn't make much sense having 
two functions parsing the exact same clause.  Perhaps add a flag to it 
and have it work in two modes: one mode to create OMP_CLAUSE_SAFELEN and 
one mode to fill the token vector.

> +              if (!c_parser_elem_fn_vectorlength (parser))
> +                {
> +                  c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, NULL);
> +                  /* NO reason to keep any of these tokens if the
> +                     vectorlength is messed up.  */
> +		  vec_free (parser->elem_fn_tokens);
> +                  return;

It may be cleaner to make the caller free the vector.

> +          /* linear and uniform are the same between SIMD
> +             enabled functions and #pragma omp declare simd.  */

Capitalize first word.

> +      /* Do not push the last ')'  */
> +      if (!(token->type == CPP_CLOSE_PAREN && paren_scope == 0))
> +	vec_safe_push (parser->elem_fn_tokens, *token);

You're documenting the obvious.  Perhaps a better comment would be to 
explain why you do not push the last ')'.

> +/* Parses the vector attribute of SIMD enabled functions in Cilk Plus.
> +   Syntax:
> +   vector
> +   vector (<vector attributes>).  */
> +
> +static void
> +c_parser_elem_fn_expr_list (c_parser *parser, c_token vec_token)

Please document the second argument.


> +  /* 2 EOF_token is added as a safety-net since the normal C front-end has
> +     two token look-ahead.  */

"Two EOF tokens are added..."

>  static void
> -expand_simd_clones (struct cgraph_node *node)
> +expand_simd_clones (struct cgraph_node *node, const char *type)

Document second argument, but perhaps we don't even need the second 
argument.  See below.

> @@ -12337,7 +12336,10 @@
>  {
>    struct cgraph_node *node;
>    FOR_EACH_FUNCTION (node)
> -    expand_simd_clones (node);
> +    expand_simd_clones (node, "omp declare simd");
> +  if (flag_enable_cilkplus)
> +    FOR_EACH_FUNCTION (node)
> +      expand_simd_clones (node, "cilk plus elemental");

First of all, it's a really bad idea to scan all the functions twice. 
You should have adapted expand_simd_clones() to do the work for both.

But even so, I don't think you need to do this at all.  Aren't Cilk Plus 
elementals supposed to be tagged as "omp declare simd" as well?  In 
which case expand_simd_clones() will DTRT.  It should...and then 
simd_clone_clauses_extract() already has the smarts to differentiate 
between the both variants.

Both OpenMP and Cilk Plus simd clones should have the "omp declare simd" 
attribute, and then Cilk Plus clones should *also* have the "cilk plus 
elemental" attribute.

Speak of which, we should probably rename the "cilk plus elemental" 
attribute throughout to "cilk plus declare simd" as I described earlier, 
but this is not your fault.  Bonus points if you want to do it as part 
of this patch :).

> +dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/EF/*.c]] " -g" " "
> +dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/EF/*.c]] " -O1" " "
> +dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/EF/*.c]] " -O2 -std=c99" " "
> +dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/EF/*.c]] " -O2 -ftree-vectorize" " "
> +dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/EF/*.c]] " -O3 -g" " "
> +

As I've mentioned before, I am really against running Cilk tests with 
all these variants.  It's time consuming for all developers and the 
chances of catching an error that is not caught with say... "-O0" are 
pretty close to 0.  Do you have any particular errors you think will be 
better checked by trying all these variants?  The time spent doesn't 
seem to be worth it.  If you have a particular test in mind, perhaps you 
should add that flag to the test itself.

For that matter, we should probably get rid of all the variant testing 
in the rest of Cilk Plus.

Furthermore, rename "EF" to something more readable and less dependent 
on "elementals" which is deprecated.  Perhaps "simd-clones" or something 
similar?

Aldy