Re: PING: Fwd: Re: [patch] implement Cilk Plus simd loops on trunk

[Jason Merrill <jason at redhat dot com>]

On 10/18/2013 02:48 PM, Aldy Hernandez wrote:
> This has the potential of throwing my mind for a spin.  Can I do this as
> a followup, and keep it simple for now?

Sure.

> > > +          else if (!TREE_TYPE (e) || !TREE_CONSTANT (e)
> > > +               || !INTEGRAL_TYPE_P (TREE_TYPE (e)))
> > > +        cp_parser_error (parser,
> > > +                 "step size must be an integer constant");
> >
> > Can't the step size be a value-dependent expression like a template
> > non-type parameter?
>
> Fixed.  I also found some nuances in how we were (NOT) handling
> variables as the standard mentioned, both for C and C++.  Fixed as well,
> and added testcases when appropriate.

> +	      else if (TREE_TYPE (e)
> +		       && INTEGRAL_TYPE_P (TREE_TYPE (e))
> +		       && (TREE_CONSTANT (e)
> +			   || DECL_P (e)))
> +		step_size = e;

This still doesn't seem like it allows arbitrary dependent 
constant-expressions.  In a template, I'm pretty sure not everything 
that will end up being a constant-expression is marked TREE_CONSTANT.

You want to check type_dependent_expression_p and 
value_dependent_expression_p.

> +  if (this_pre_body)
> +    {
> +      this_pre_body = pop_stmt_list (this_pre_body);
> +      if (*pre_body)
> +	{
> +	  tree t = *pre_body;
> +	  *pre_body = push_stmt_list ();
> +	  add_stmt (t);
> +	  add_stmt (this_pre_body);
> +	  *pre_body = pop_stmt_list (*pre_body);
> +	}
> +      else
> +	*pre_body = this_pre_body;
> +    }

I think you can just use

append_to_statement_list (this_pre_body, pre_body)

> +  if (for_keyword == RID_FOR)
> +    decl = cp_parser_simd_for_init_statement (parser, &init, &pre_body);
> +
> +  if (decl == error_mark_node)

Don't you get a warning about decl possibly being used uninitialized 
here?  I think you want

else
  /* FIXME when _Cilk_for is implemented.  */
  decl = NULL_TREE;

I guess the assert previously protected you from the uninitialized warning.

> +  if (cp_lexer_next_token_is (parser->lexer, CPP_COMMA))
> +    {
> +      error_at (loc, "%s-loop initializer cannot have multiple variable "
> +		"declarations", for_keyword == RID_FOR ? "for" : "_Cilk_for");
> +      cp_parser_skip_to_end_of_statement (parser);
> +      valid = false;
> +    }
> +
> +  if (!valid)
> +    {
> +      /* Skip to the semicolon ending the init.  */
> +      cp_parser_skip_to_end_of_statement (parser);
> +    }
>
> +  if (!cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON))
> +    return error_mark_node;

I think this should move into cp_parser_simd_for_init_statement or 
cp_parser_omp_for_loop_init.

Out of curiosity, how are OpenMP and Cilk+ for loops so different that 
they can't share a parsing function?

> +/* Adjust any clauses to match the requirements for OpenMP.  */

I guess the idea here is that the clauses start out as Cilk+ clauses? 
The comment should make that clearer.

> -  if (flag_openmp == 0)
> +  if (!flag_openmp && !flag_enable_cilkplus)

Seems like there should be an internal flag to enable the common internals.

> +/* Cilk Plus - #pragma simd [clause1 ... clauseN]
> +   Operands like for OMP_FOR.  */
> +DEFTREECODE (CILK_SIMD, "cilk_simd", tcc_statement, 6)

Could this just be a flag on OMP_SIMD?

Jason