Message ID | 562A95C3.2040100@mentor.com |
---|---|
State | New |
Headers | show |
On Fri, Oct 23, 2015 at 01:17:07PM -0700, Cesar Philippidis wrote: > Good idea, thanks. This patch also corrects the problems parsing weird > combinations of num, static and length arguments that you mentioned > elsewhere. > > Is this OK for trunk? I'd strongly prefer to see always patches accompanied by testcases. > + loc = c_parser_peek_token (parser)->location; > + op_to_parse = &op0; > + > + if ((c_parser_next_token_is (parser, CPP_NAME) > + || c_parser_next_token_is (parser, CPP_KEYWORD)) > + && c_parser_peek_2nd_token (parser)->type == CPP_COLON) > + { > + tree name_kind = c_parser_peek_token (parser)->value; > + const char *p = IDENTIFIER_POINTER (name_kind); I think I'd prefer not to peek at this at all if it is RID_STATIC, so perhaps just have (and name_kind is weird): else { tree val = c_parser_peek_token (parser)->value; if (strcmp (id, IDENTIFIER_POINTER (val)) == 0) { c_parser_consume_token (parser); /* id */ c_parser_consume_token (parser); /* ':' */ } else { ... } } ? > + if (kind == OMP_CLAUSE_GANG > + && c_parser_next_token_is_keyword (parser, RID_STATIC)) > + { > + c_parser_consume_token (parser); /* static */ > + c_parser_consume_token (parser); /* ':' */ > + > + op_to_parse = &op1; > + if (c_parser_next_token_is (parser, CPP_MULT)) > + { > + c_parser_consume_token (parser); > + *op_to_parse = integer_minus_one_node; > + > + /* Consume a comma if present. */ > + if (c_parser_next_token_is (parser, CPP_COMMA)) > + c_parser_consume_token (parser); Doesn't this mean that you happily parse gang (static: * abc) or gang (static:*num:1) etc.? I'd say the comma should be non-optional (i.e. either accept CPP_COMMA, or CPP_CLOSE_PARENT, but nothing else) in that case (at least, when in OpenMP grammar something is *-list it is meant to be comma separated). > + /* Consume a comma if present. */ > + if (c_parser_next_token_is (parser, CPP_COMMA)) > + c_parser_consume_token (parser); Similarly this means gang (num: 5 static: *) is accepted. If it is valid, then again it should have testsuite coverage. Jakub
On Fri, Oct 23, 2015 at 10:31:55PM +0200, Jakub Jelinek wrote: > Doesn't this mean that you happily parse > gang (static: * abc) > or > gang (static:*num:1) > etc.? I'd say the comma should be non-optional (i.e. either accept > CPP_COMMA, or CPP_CLOSE_PARENT, but nothing else) in that case (at least, > when in OpenMP grammar something is *-list it is meant to be comma > separated). Looking at the OpenACC standard, gang-arg-list is indeed a comma separated list of gang-arg, so the above are not valid, so you really should just error out and skip to close paren if at that spot isn't a CPP_COMMA or CPP_CLOSE_PAREN. And for vector/worker arguments, which don't accept a *-list, IMNSHO you shouldn't even try to accept CPP_COMMA, just require CPP_CLOSE_PAREN. > > > + /* Consume a comma if present. */ > > + if (c_parser_next_token_is (parser, CPP_COMMA)) > > + c_parser_consume_token (parser); > > Similarly this means > gang (num: 5 static: *) > is accepted. If it is valid, then again it should have testsuite coverage. Jakub
On 10/23/15 16:17, Cesar Philippidis wrote: > Nathan, can you try out this patch with your updated patch set? I saw > some test cases getting stuck when expanding expand_GOACC_DIM_SIZE in on > the host compiler, which is wrong. I don't see that happening in > gomp-4_0-branch with this patch. Also, can you merge this patch along > with the c++ and new test case patches to trunk? I'll handle the gomp4 > backport. Wilco. nathan
On 10/23/2015 01:31 PM, Jakub Jelinek wrote: > On Fri, Oct 23, 2015 at 01:17:07PM -0700, Cesar Philippidis wrote: >> Good idea, thanks. This patch also corrects the problems parsing weird >> combinations of num, static and length arguments that you mentioned >> elsewhere. >> >> Is this OK for trunk? > > I'd strongly prefer to see always patches accompanied by testcases. > >> + loc = c_parser_peek_token (parser)->location; >> + op_to_parse = &op0; >> + >> + if ((c_parser_next_token_is (parser, CPP_NAME) >> + || c_parser_next_token_is (parser, CPP_KEYWORD)) >> + && c_parser_peek_2nd_token (parser)->type == CPP_COLON) >> + { >> + tree name_kind = c_parser_peek_token (parser)->value; >> + const char *p = IDENTIFIER_POINTER (name_kind); > > I think I'd prefer not to peek at this at all if it is RID_STATIC, > so perhaps just have (and name_kind is weird): > else > { > tree val = c_parser_peek_token (parser)->value; > if (strcmp (id, IDENTIFIER_POINTER (val)) == 0) > { > c_parser_consume_token (parser); /* id */ > c_parser_consume_token (parser); /* ':' */ > } > else > { > ... > } > } > ? My plan over here was try and catch any arguments with a colon. But that fell threw because... >> + if (kind == OMP_CLAUSE_GANG >> + && c_parser_next_token_is_keyword (parser, RID_STATIC)) >> + { >> + c_parser_consume_token (parser); /* static */ >> + c_parser_consume_token (parser); /* ':' */ >> + >> + op_to_parse = &op1; >> + if (c_parser_next_token_is (parser, CPP_MULT)) >> + { >> + c_parser_consume_token (parser); >> + *op_to_parse = integer_minus_one_node; >> + >> + /* Consume a comma if present. */ >> + if (c_parser_next_token_is (parser, CPP_COMMA)) >> + c_parser_consume_token (parser); > > Doesn't this mean that you happily parse > gang (static: * abc) > or > gang (static:*num:1) > etc.? I'd say the comma should be non-optional (i.e. either accept > CPP_COMMA, or CPP_CLOSE_PARENT, but nothing else) in that case (at least, > when in OpenMP grammar something is *-list it is meant to be comma > separated). I'm not handling commas properly. My next patch is going to handle the static argument separately. >> + /* Consume a comma if present. */ >> + if (c_parser_next_token_is (parser, CPP_COMMA)) >> + c_parser_consume_token (parser); > > Similarly this means > gang (num: 5 static: *) > is accepted. If it is valid, then again it should have testsuite coverage. I'll include a test case for this with the next patch. Cesar
On 10/23/15 17:25, Nathan Sidwell wrote: > On 10/23/15 16:17, Cesar Philippidis wrote: > >> Nathan, can you try out this patch with your updated patch set? I saw >> some test cases getting stuck when expanding expand_GOACC_DIM_SIZE in on >> the host compiler, which is wrong. I don't see that happening in >> gomp-4_0-branch with this patch. Also, can you merge this patch along >> with the c++ and new test case patches to trunk? I'll handle the gomp4 >> backport. > > Wilco. testing your patch on trunk along with my IFN_UNIQUE changes shows good results. nathan
2015-10-20 Cesar Philippidis <cesar@codesourcery.com> Thomas Schwinge <thomas@codesourcery.com> James Norris <jnorris@codesourcery.com> Joseph Myers <joseph@codesourcery.com> Julian Brown <julian@codesourcery.com> Bernd Schmidt <bschmidt@redhat.com> * c-parser.c (c_parser_oacc_shape_clause): New. (c_parser_oacc_simple_clause): New. (c_parser_oacc_all_clauses): Add auto, gang, seq, vector, worker. (OACC_LOOP_CLAUSE_MASK): Add gang, worker, vector, auto, seq. diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index c8c6a2d..1e3c333 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -11188,6 +11188,142 @@ c_parser_omp_clause_num_workers (c_parser *parser, tree list) } /* OpenACC: + + gang [( gang-arg-list )] + worker [( [num:] int-expr )] + vector [( [length:] int-expr )] + + where gang-arg is one of: + + [num:] int-expr + static: size-expr + + and size-expr may be: + + * + int-expr +*/ + +static tree +c_parser_oacc_shape_clause (c_parser *parser, omp_clause_code kind, + const char *str, tree list) +{ + const char *id = "num"; + + if (kind == OMP_CLAUSE_VECTOR) + id = "length"; + + tree op0 = NULL_TREE, op1 = NULL_TREE; + location_t loc = c_parser_peek_token (parser)->location; + + if (c_parser_next_token_is (parser, CPP_OPEN_PAREN)) + { + tree *op_to_parse = &op0; + c_parser_consume_token (parser); + + do + { + loc = c_parser_peek_token (parser)->location; + op_to_parse = &op0; + + if ((c_parser_next_token_is (parser, CPP_NAME) + || c_parser_next_token_is (parser, CPP_KEYWORD)) + && c_parser_peek_2nd_token (parser)->type == CPP_COLON) + { + tree name_kind = c_parser_peek_token (parser)->value; + const char *p = IDENTIFIER_POINTER (name_kind); + if (kind == OMP_CLAUSE_GANG + && c_parser_next_token_is_keyword (parser, RID_STATIC)) + { + c_parser_consume_token (parser); /* static */ + c_parser_consume_token (parser); /* ':' */ + + op_to_parse = &op1; + if (c_parser_next_token_is (parser, CPP_MULT)) + { + c_parser_consume_token (parser); + *op_to_parse = integer_minus_one_node; + + /* Consume a comma if present. */ + if (c_parser_next_token_is (parser, CPP_COMMA)) + c_parser_consume_token (parser); + + continue; + } + } + else if (strcmp (id, p) == 0) + { + c_parser_consume_token (parser); /* id */ + c_parser_consume_token (parser); /* ':' */ + } + else + { + if (kind == OMP_CLAUSE_GANG) + c_parser_error (parser, "expected %<num%> or %<static%>"); + else if (kind == OMP_CLAUSE_VECTOR) + c_parser_error (parser, "expected %<length%>"); + else + c_parser_error (parser, "expected %<num%>"); + c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, 0); + return list; + } + } + + if (*op_to_parse != NULL_TREE) + { + c_parser_error (parser, "unexpected argument"); + c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, 0); + return list; + } + + tree expr = c_parser_expr_no_commas (parser, NULL).value; + if (expr == error_mark_node) + { + c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, 0); + return list; + } + + mark_exp_read (expr); + *op_to_parse = expr; + + /* Consume a comma if present. */ + if (c_parser_next_token_is (parser, CPP_COMMA)) + c_parser_consume_token (parser); + } + while (!c_parser_next_token_is (parser, CPP_CLOSE_PAREN)); + c_parser_consume_token (parser); + } + + check_no_duplicate_clause (list, kind, str); + + tree c = build_omp_clause (loc, kind); + if (op0) + OMP_CLAUSE_OPERAND (c, 0) = op0; + if (op1) + OMP_CLAUSE_OPERAND (c, 1) = op1; + OMP_CLAUSE_CHAIN (c) = list; + return c; +} + +/* OpenACC: + auto + independent + nohost + seq */ + +static tree +c_parser_oacc_simple_clause (c_parser *parser ATTRIBUTE_UNUSED, + enum omp_clause_code code, tree list) +{ + check_no_duplicate_clause (list, code, omp_clause_code_name[code]); + + tree c = build_omp_clause (c_parser_peek_token (parser)->location, code); + OMP_CLAUSE_CHAIN (c) = list; + + return c; +} + +/* OpenACC: async [( int-expr )] */ static tree @@ -12393,6 +12529,11 @@ c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask, clauses = c_parser_oacc_clause_async (parser, clauses); c_name = "async"; break; + case PRAGMA_OACC_CLAUSE_AUTO: + clauses = c_parser_oacc_simple_clause (parser, OMP_CLAUSE_AUTO, + clauses); + c_name = "auto"; + break; case PRAGMA_OACC_CLAUSE_COLLAPSE: clauses = c_parser_omp_clause_collapse (parser, clauses); c_name = "collapse"; @@ -12429,6 +12570,11 @@ c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask, clauses = c_parser_omp_clause_firstprivate (parser, clauses); c_name = "firstprivate"; break; + case PRAGMA_OACC_CLAUSE_GANG: + c_name = "gang"; + clauses = c_parser_oacc_shape_clause (parser, OMP_CLAUSE_GANG, + c_name, clauses); + break; case PRAGMA_OACC_CLAUSE_HOST: clauses = c_parser_oacc_data_clause (parser, c_kind, clauses); c_name = "host"; @@ -12477,6 +12623,16 @@ c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask, clauses = c_parser_oacc_data_clause (parser, c_kind, clauses); c_name = "self"; break; + case PRAGMA_OACC_CLAUSE_SEQ: + clauses = c_parser_oacc_simple_clause (parser, OMP_CLAUSE_SEQ, + clauses); + c_name = "seq"; + break; + case PRAGMA_OACC_CLAUSE_VECTOR: + c_name = "vector"; + clauses = c_parser_oacc_shape_clause (parser, OMP_CLAUSE_VECTOR, + c_name, clauses); + break; case PRAGMA_OACC_CLAUSE_VECTOR_LENGTH: clauses = c_parser_omp_clause_vector_length (parser, clauses); c_name = "vector_length"; @@ -12485,6 +12641,11 @@ c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask, clauses = c_parser_oacc_clause_wait (parser, clauses); c_name = "wait"; break; + case PRAGMA_OACC_CLAUSE_WORKER: + c_name = "worker"; + clauses = c_parser_oacc_shape_clause (parser, OMP_CLAUSE_WORKER, + c_name, clauses); + break; default: c_parser_error (parser, "expected %<#pragma acc%> clause"); goto saw_error; @@ -13015,6 +13176,11 @@ c_parser_oacc_enter_exit_data (c_parser *parser, bool enter) #define OACC_LOOP_CLAUSE_MASK \ ( (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_COLLAPSE) \ + | (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_GANG) \ + | (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_WORKER) \ + | (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_VECTOR) \ + | (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_AUTO) \ + | (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_SEQ) \ | (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_REDUCTION) ) static tree