Re: [gomp4/cilk-in-gomp/cilkplus] C++ parsing for Cilk plus <#pragma simd>

[Aldy Hernandez <aldyh at redhat dot com>]

On 05/21/13 10:46, Iyer, Balaji V wrote:

Thank you for working on this!

> static tree
> find_invalid_stmts (tree *tp, int *walk_subtrees, void *data)
> {
>   bool *valid = (bool *) data;
>   location_t loc = EXPR_HAS_LOCATION (*tp) ? EXPR_LOCATION (*tp) :
>     UNKNOWN_LOCATION;
>   if (!tp || !*tp)
>     return NULL_TREE;
>   else if (TREE_CODE (*tp) == GOTO_EXPR)
>     {
>       error_at (loc, "goto statements are not allowed inside "
>                 "loops marked with #pragma simd");
>       *valid = false;
>       *walk_subtrees = 0;
>     }

As I've said in the comment to the analogous C function 
(find_invalid_stmts_in_loops), you can't just check for GOTO_EXPRs, 
since they can also be generated by switches and other loop constructs. 
 I will later add some generic code to check this sometime after the 
CFG is built.  For now, you can remove the check for GOTO_EXPR.

>   else if (TREE_CODE (*tp) == CALL_EXPR)
>     {
>       tree fndecl = CALL_EXPR_FN (*tp);
>
>       if (TREE_CODE (fndecl) == ADDR_EXPR)
>         fndecl = TREE_OPERAND (fndecl, 0);
>       if (TREE_CODE (fndecl) == FUNCTION_DECL)
>         {
>           if (setjmp_call_p (fndecl))
>             {
>               error_at (loc, "setjmps are not allowed inside loops marked with"
>                         " #pragma simd");
>               *valid = false;
>               *walk_subtrees = 0;
>             }
>         }

This is already checked in the C counterpart.  Can't you just call 
find_invalid_stmts_in_loops() from find_invalid_stmts since it checks 
for everything C/C++ related?  Ideally your find_invalid_stmts() 
function should only check the C++ specific stuff (throw, try, etc) and 
call find_invalid_stmts_in_loops.


>  else if (TREE_CODE (*tp) == THROW_EXPR)
>     {
>       error_at (loc, "throw expressions are not allowed inside the loop "
>                 "marked with pragma simd");

s/the loop/loops

>   else if (TREE_CODE (*tp) == TRY_BLOCK)
>     {
>       error_at (loc, "try statements are not allowed inside loop marked with "

s/loop/loops

> bool
> p_simd_valid_stmts_in_body_p (tree body)

Is there a reason for the p_ prefix?  If not, could you remove it?

> @@ -29458,6 +29472,7 @@ cp_parser_initial_pragma (cp_token *first_token)
>    cp_lexer_get_preprocessor_token (NULL, first_token);
>  }
>
> +
>  /* Normal parsing of a pragma token.  Here we can (and must) use the
>     regular lexer.  */

Whitespace.

> @@ -29605,6 +29620,11 @@ cp_parser_pragma (cp_parser *parser, enum pragma_context context)
>  		"%<#pragma omp sections%> construct");
>        break;
>
> +    case PRAGMA_CILK_SIMD:
> +      if (context == pragma_external)
> +	goto bad_stmt;
> +      cp_parser_cilk_simd_construct (parser, pragma_tok);
> +      return true;
>      default:

Please add an empty line after the return, since the previous code had a 
space there.

Can you put all the cp_parser_cilk_for* functions before 
cp_parser_cilk_simd_vectorlength and avoid the prototype for 
cp_parser_cilk_for?  Just curious, I don't know if there are any 
dependencies I'm not seeing.

> +/* Main entry-point for parsing Cilk Plus <#pragma simd> for loop.  */

s/loop/loops

> +  /* #pragma simd is build on top of OpenMP 4.0's OMP_SIMD treees.  Thus
> +     openmp must be enabled.  */

s/build/built
s/treees/trees
s/openmp/OpenMP

> +/* Returns the name of the next clause.  If the clause is not recognized, then
> +   PRAGMA_CILK_CLAUSE_NONE is returned and the next token is not consumed.
> +   Otherwise, the appropriate enum. value from the pragma_simd_clause is
> +   returned and the token is consumed.  */
> +
> +static pragma_cilk_clause
> +cp_parser_cilk_simd_clause_name (cp_parser *parser, tree *name)


Replace "enum. value" with "enum".

Also, you are not documenting the NAME argument.

> +  /* Vectorlength clause behaves exactly like Open MP 4.0's safelen clause.

s/Vectorlength/The vectorlength

Regarding the tests, I will merge trunk->cilk-in-gomp so we can reuse 
the c-c++-common infrastructure you wrote.  There is no sense having 
multiple tests for vectorlength, linear, etc.

Aldy