On Wed, Oct 21, 2015 at 03:18:55PM -0400, Nathan Sidwell wrote:
> This patch is the C++ changes matching the C ones of patch 4. In
> finish_omp_clauses, the gang, worker, & vector clauses are handled the same
> as OpenMP's 'num_threads' clause. One change to num_threads is the
> augmentation of a diagnostic to add %<...%> markers to the clause name.
Indeed, lots of older OpenMP diagnostics is missing %<...%> markers around
keywords. Something to fix eventually.
> 2015-10-20 Cesar Philippidis <ce...@codesourcery.com>
> Thomas Schwinge <tho...@codesourcery.com>
> James Norris <jnor...@codesourcery.com>
> Joseph Myers <jos...@codesourcery.com>
> Julian Brown <jul...@codesourcery.com>
> Nathan Sidwell <nat...@codesourcery.com>
>
> * parser.c (cp_parser_omp_clause_name): Add auto, gang, seq,
> vector, worker.
> (cp_parser_oacc_simple_clause): New.
> (cp_parser_oacc_shape_clause): New.
What I've said for the C FE patch, plus:
> + if (cp_lexer_next_token_is (lexer, CPP_NAME)
> + || cp_lexer_next_token_is (lexer, CPP_KEYWORD))
> + {
> + tree name_kind = cp_lexer_peek_token (lexer)->u.value;
> + const char *p = IDENTIFIER_POINTER (name_kind);
> + if (kind == OMP_CLAUSE_GANG && strcmp ("static", p) == 0)
As static is a keyword, wouldn't it be better to just handle that case
using cp_lexer_next_token_is_keyword (lexer, RID_STATIC)?
Also, what is the exact grammar of the shape arguments?
Would be nice to describe the grammar, in the grammar you just say
expression, at least for vector/worker, which is clearly not accurate.
It seems the intent is that num: or length: or static: is optional, right?
But if that is the case, you should treat those as parsed only if followed
by :. While static is a keyword, so you can't have a variable called like
that, having vector(length) or vector(num) should not be rejected.
So, I would have expected that it should test if it is RID_STATIC
followed by CPP_COLON (and only in that case consume those tokens),
or CPP_NAME of id followed by CPP_COLON (and only in that case consume those
tokens), otherwise parse it as assignment expression.
The C FE may have similar issue. Plus of course there should be testsuite
coverage for all the weird cases.
> + case OMP_CLAUSE_GANG:
> + case OMP_CLAUSE_VECTOR:
> + case OMP_CLAUSE_WORKER:
> + /* Operand 0 is the num: or length: argument. */
> + t = OMP_CLAUSE_OPERAND (c, 0);
> + if (t == NULL_TREE)
> + break;
> +
> + t = maybe_convert_cond (t);
Can you explain the maybe_convert_cond calls (in both cases here,
plus the preexisting in OMP_CLAUSE_VECTOR_LENGTH)?
The reason why it is used for OpenMP if and final clauses is that those have
a condition argument, either the condition is zero or non-zero (so
effectively it is turned into a bool).
But aren't the gang/vector/worker/vector_length arguments integers rather
than conditions? I'd expect that finish_omp_clauses should verify
those operands are indeed integral expressions (if that is the requirement
in the standard), as it is something that for C++ can't be verified during
parsing, if arbitrary expressions are parsed there.
> @@ -5959,32 +5990,58 @@ finish_omp_clauses (tree clauses, bool a
> break;
>
> case OMP_CLAUSE_NUM_THREADS:
> - t = OMP_CLAUSE_NUM_THREADS_EXPR (c);
> - if (t == error_mark_node)
> - remove = true;
> - else if (!type_dependent_expression_p (t)
> - && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
> - {
> - error ("num_threads expression must be integral");
> - remove = true;
> - }
> - else
> - {
> - t = mark_rvalue_use (t);
> - if (!processing_template_decl)
> - {
> - t = maybe_constant_value (t);
> - if (TREE_CODE (t) == INTEGER_CST
> - && tree_int_cst_sgn (t) != 1)
> - {
> - warning_at (OMP_CLAUSE_LOCATION (c), 0,
> - "%<num_threads%> value must be positive");
> - t = integer_one_node;
> - }
> - t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
> - }
> - OMP_CLAUSE_NUM_THREADS_EXPR (c) = t;
> - }
> + case OMP_CLAUSE_NUM_GANGS:
> + case OMP_CLAUSE_NUM_WORKERS:
> + case OMP_CLAUSE_VECTOR_LENGTH:
If you are already merging some of the similar handling, please
handle OMP_CLAUSE_NUM_TEAMS and OMP_CLAUSE_NUM_TASKS the same way.
Jakub
