[OpenACC] num_gangs, num_workers and vector_length in c++

I noticed that num_gangs, num_workers and vector_length are parsed in
somewhat insistent ways in the c++ FE. Both vector_length and num_gangs
bail out whenever as soon as they detect errors, whereas num_workers
does not. Besides for that, they are also checking for integral
expressions as the arguments are scanned instead of deferring that check
to finish_omp_clauses. That check will cause ICEs when template
arguments are used when we add support for template arguments later on.

Rather than fix each function individually, I've consolidated them into
a single cp_parser_oacc_positive_int_clause function. While this
function could be extended to support openmp clauses which accept an
integer expression argument, like num_threads, I've decided to leave
those as-is since there are no known problems with those functions at
this moment.

It this OK for trunk? I've regression tested and bootstrapped on
x86_64-linux.

Cesar

On Thu, Oct 29, 2015 at 04:02:11PM -0700, Cesar Philippidis wrote:
> I noticed that num_gangs, num_workers and vector_length are parsed in
> somewhat insistent ways in the c++ FE. Both vector_length and num_gangs
> bail out whenever as soon as they detect errors, whereas num_workers
> does not. Besides for that, they are also checking for integral
> expressions as the arguments are scanned instead of deferring that check
> to finish_omp_clauses. That check will cause ICEs when template
> arguments are used when we add support for template arguments later on.
> 
> Rather than fix each function individually, I've consolidated them into
> a single cp_parser_oacc_positive_int_clause function. While this
> function could be extended to support openmp clauses which accept an
> integer expression argument, like num_threads, I've decided to leave
> those as-is since there are no known problems with those functions at
> this moment.

First question is what int-expr in OpenACC actually stands for (but I'll
have to raise similar question for OpenMP too).

Previously you were using cp_parser_condition, which is clearly undesirable
in this case, it allows e.g.
num_gangs (int a = 5)
but the question is if
num_gangs (5, 6)
is valid and stands for (5, 6) expression, then it should use
cp_parser_expression, or if you want to error on it, then you should use
cp_parser_assignment_expression.
From quick skimming of the (now removed) C/C++ Grammar Appendix in OpenMP,
I believe all the places where expression or scalar-expression is used
in the grammar are meant to be cp_parser_expression cases (except
expression-list used in UDRs which stands for normal C++ expression-list
non-terminal), so clearly I need to fix up omp_clause_{if,final} to call
cp_parser_expression instead of cp_parser_condition, and the various
OpenMP clauses that use cp_parser_assignment_expression to instead use
cp_parser_expression.  Say schedule(static, 3, 6) should be valid IMHO.
But, in OpenMP expression or scalar-expression in the grammar is never
followed by , or optional , while in OpenACC grammar clearly is (e.g. for
the gang clause).
If OpenACC wants something different, clearly you can't share the parsing
routines between say num_tasks and num_workers.

Another thing is what Jason as C++ maintainer wants, it is nice to get rid
of some code redundancies, on the other side the fact that there is one
function per non-terminal in the grammar is also quite nice property.
I know I've violated this a few times too.

Next question is, why do you call it cp_parser_oacc_positive_int_clause
when the parsing function actually doesn't verify neither the positive nor
the int properties (and it should not), so perhaps it should just reflect
in the name that it is a clause with assignment? expression.
Or, see the previous paragraph, have a helper that does that and then
have a separate function for each clause kind that calls those with the
right arguments.

	Jakub

On 10/30/2015 06:37 AM, Jakub Jelinek wrote:
> On Thu, Oct 29, 2015 at 04:02:11PM -0700, Cesar Philippidis wrote:
>> I noticed that num_gangs, num_workers and vector_length are parsed in
>> somewhat insistent ways in the c++ FE. Both vector_length and num_gangs
>> bail out whenever as soon as they detect errors, whereas num_workers
>> does not. Besides for that, they are also checking for integral
>> expressions as the arguments are scanned instead of deferring that check
>> to finish_omp_clauses. That check will cause ICEs when template
>> arguments are used when we add support for template arguments later on.
>>
>> Rather than fix each function individually, I've consolidated them into
>> a single cp_parser_oacc_positive_int_clause function. While this
>> function could be extended to support openmp clauses which accept an
>> integer expression argument, like num_threads, I've decided to leave
>> those as-is since there are no known problems with those functions at
>> this moment.
> 
> First question is what int-expr in OpenACC actually stands for (but I'll
> have to raise similar question for OpenMP too).
> 
> Previously you were using cp_parser_condition, which is clearly undesirable
> in this case, it allows e.g.
> num_gangs (int a = 5)
> but the question is if
> num_gangs (5, 6)
> is valid and stands for (5, 6) expression, then it should use
> cp_parser_expression, or if you want to error on it, then you should use
> cp_parser_assignment_expression.

The openacc spec doesn't actually define int-expr, but we take to me
mean a single integral value. In general, the openacc spec uses the term
list to describe comma separated expressions. So we've been assuming
that expr cannot contain commas. Besides, for num_gangs, num_workers and
vector_length it doesn't make sense to accept more than one value. A
construct can accept one than one of those clauses, but they'd have to
be associated with a different device_type.

> From quick skimming of the (now removed) C/C++ Grammar Appendix in OpenMP,
> I believe all the places where expression or scalar-expression is used
> in the grammar are meant to be cp_parser_expression cases (except
> expression-list used in UDRs which stands for normal C++ expression-list
> non-terminal), so clearly I need to fix up omp_clause_{if,final} to call
> cp_parser_expression instead of cp_parser_condition, and the various
> OpenMP clauses that use cp_parser_assignment_expression to instead use
> cp_parser_expression.  Say schedule(static, 3, 6) should be valid IMHO.
> But, in OpenMP expression or scalar-expression in the grammar is never
> followed by , or optional , while in OpenACC grammar clearly is (e.g. for
> the gang clause).
> If OpenACC wants something different, clearly you can't share the parsing
> routines between say num_tasks and num_workers.

So num_threads, num_tasks, grainsize, priority, hint, num_teams,
thread_limit should all accept comma-separated lists?

> Another thing is what Jason as C++ maintainer wants, it is nice to get rid
> of some code redundancies, on the other side the fact that there is one
> function per non-terminal in the grammar is also quite nice property.
> I know I've violated this a few times too.
> 
> Next question is, why do you call it cp_parser_oacc_positive_int_clause
> when the parsing function actually doesn't verify neither the positive nor
> the int properties (and it should not), so perhaps it should just reflect
> in the name that it is a clause with assignment? expression.
> Or, see the previous paragraph, have a helper that does that and then
> have a separate function for each clause kind that calls those with the
> right arguments.

That name had some legacy from the c FE in gomp-4_0-branch which the
function was inherited from. On one hand, it doesn't make sense to allow
negative integer values for those clauses, but at the same time, those
values aren't checked during scanning. Maybe it should be renamed
cp_parser_oacc_single_int_clause instead?

If you like, I could make a more general
cp_parser_omp_generic_expression that has a scan_list argument so that
it can be used for both general expressions and assignment-expressions.
That way it can be used for both omp and oacc clauses of the form 'foo (
expression )'.

What's your preference?

Thanks,
Cesar

On Fri, Oct 30, 2015 at 07:42:39AM -0700, Cesar Philippidis wrote:
> The openacc spec doesn't actually define int-expr, but we take to me
> mean a single integral value. In general, the openacc spec uses the term
> list to describe comma separated expressions. So we've been assuming

So does OpenMP use *-list, except one place in the grammar where
expression-list is used, but that is to describe function call arguments 
and the C++ expression-list non-terminal is essentially
{assignment-expression}-list.

> that expr cannot contain commas. Besides, for num_gangs, num_workers and
> vector_length it doesn't make sense to accept more than one value. A

Well, the comma expression are not a way to supply two values, but to
evaluate some side-effects before evaluating the expression you want.

> > From quick skimming of the (now removed) C/C++ Grammar Appendix in OpenMP,
> > I believe all the places where expression or scalar-expression is used
> > in the grammar are meant to be cp_parser_expression cases (except
> > expression-list used in UDRs which stands for normal C++ expression-list
> > non-terminal), so clearly I need to fix up omp_clause_{if,final} to call
> > cp_parser_expression instead of cp_parser_condition, and the various
> > OpenMP clauses that use cp_parser_assignment_expression to instead use
> > cp_parser_expression.  Say schedule(static, 3, 6) should be valid IMHO.
> > But, in OpenMP expression or scalar-expression in the grammar is never
> > followed by , or optional , while in OpenACC grammar clearly is (e.g. for
> > the gang clause).
> > If OpenACC wants something different, clearly you can't share the parsing
> > routines between say num_tasks and num_workers.
> 
> So num_threads, num_tasks, grainsize, priority, hint, num_teams,
> thread_limit should all accept comma-separated lists?

They accept expression, which is e.g. for C++:
expression:
	assignment-expression
	expression , assignment-expression

> 
> > Another thing is what Jason as C++ maintainer wants, it is nice to get rid
> > of some code redundancies, on the other side the fact that there is one
> > function per non-terminal in the grammar is also quite nice property.
> > I know I've violated this a few times too.

> That name had some legacy from the c FE in gomp-4_0-branch which the
> function was inherited from. On one hand, it doesn't make sense to allow
> negative integer values for those clauses, but at the same time, those
> values aren't checked during scanning. Maybe it should be renamed
> cp_parser_oacc_single_int_clause instead?

That is better.

> If you like, I could make a more general
> cp_parser_omp_generic_expression that has a scan_list argument so that
> it can be used for both general expressions and assignment-expressions.
> That way it can be used for both omp and oacc clauses of the form 'foo (
> expression )'.

No, that will only confuse readers of the parser.  After all, the code to
parse an expression argument of a clause is not that large.
So, either cp_parser_oacc_single_int_clause or just keeping the old separate
parsing functions, just remove the cruft from those (testing the type,
using cp_parser_condition instead of cp_parser_assignment_expression) is ok
with me.  Please ping Jason on what he prefers from those two.

	Jakub