This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On 07/09/2015 11:53 AM, Jakub Jelinek wrote:
Hi! On Thu, Jul 09, 2015 at 11:24:44AM -0700, Aldy Hernandez wrote: Thanks for working on it.+ wide_int offset = wi::neg (addend, &overflow); + addend = wide_int_to_tree (TREE_TYPE (addend), offset); + if (overflow) + warning_at (c_parser_peek_token (parser)->location, + OPT_Woverflow, + "possible overflow in %<depend(sink)%> offset");possible overflow looks weird. Shouldn't it complain the same as it does if you do: int c = - (-2147483648);
Done.
?--- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -12489,6 +12489,11 @@ c_finish_omp_clauses (tree clauses, bool declare_simd) == OMP_CLAUSE_DEPEND_SOURCE); break; } + if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK) + { + gcc_assert (TREE_CODE (t) == TREE_LIST); + break; + } if (TREE_CODE (t) == TREE_LIST) { if (handle_omp_array_sections (c))Won't this ICE if somebody uses depend(sink:) ? or depend(sink:.::) or similar garbage? Make sure you don't create OMP_CLAUSE_DEPEND in that case.
I've fixed the parser to avoid creating such clause.
diff --git a/gcc/gimple-walk.c b/gcc/gimple-walk.c index f0e2c67..ba79977 100644 --- a/gcc/gimple-walk.c +++ b/gcc/gimple-walk.c @@ -327,6 +327,10 @@ walk_gimple_op (gimple stmt, walk_tree_fn callback_op, } break; + case GIMPLE_OMP_ORDERED: + /* Ignore clauses. */ + break; +I'm not convinced you don't want to walk the clauses.
Ok, I've done so.Note that the OMP_CLAUSE_DECL will contain a TREE_LIST whose TREE_PURPOSE had the variable. I noticed that walking TREE_LIST's just walks the TREE_VALUE, not the TREE_PURPOSE:
case TREE_LIST: WALK_SUBTREE (TREE_VALUE (*tp)); WALK_SUBTREE_TAIL (TREE_CHAIN (*tp)); break;So, I changed the layout of the OMP_CLAUSE_DECL TREE_LIST to have the variable in the TREE_VALUE. The TREE_PURPOSE will contain the lone integer, which shouldn't need to be walked. However, if later (C++ iterators??) we have a TREE_PURPOSE that needs to be walked we will have to change the walker or the layout.
diff --git a/gcc/gimple.h b/gcc/gimple.h index 6057ea0..e33fe1e 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -527,6 +527,17 @@ struct GTY((tag("GSS_OMP_CRITICAL"))) tree name; }; +/* GIMPLE_OMP_ORDERED */ + +struct GTY((tag("GSS_OMP_ORDERED"))) + gomp_ordered : public gimple_statement_omp +{ + /* [ WORD 1-7 ] : base class */ + + /* [ WORD 8 ] */ + tree clauses; +};I would have expected to use struct GTY((tag("GSS_OMP_SINGLE_LAYOUT"))) gomp_ordered : public gimple_statement_omp_single_layout { /* No extra fields; adds invariant: stmt->code == GIMPLE_OMP_ORDERED. */ }; instead (like gomp_single, gomp_teams, ...).
Oh, neat. I missed that. Fixed.
@@ -149,6 +149,9 @@ struct gimplify_omp_ctx struct gimplify_omp_ctx *outer_context; splay_tree variables; hash_set<tree> *privatized_types; + /* Iteration variables in an OMP_FOR. */ + tree *iter_vars; + int niter_vars;Wonder if it wouldn't be better to use a vec<tree> instead. Then the size would be there as vec_length.
Done.
@@ -8169,6 +8185,19 @@ gimplify_transaction (tree *expr_p, gimple_seq *pre_p) return GS_ALL_DONE; } +/* Verify the validity of the depend(sink:...) variable VAR. + Return TRUE if everything is OK, otherwise return FALSE. */ + +static bool +verify_sink_var (location_t loc, tree var) +{ + for (int i = 0; i < gimplify_omp_ctxp->niter_vars; ++i) + if (var == gimplify_omp_ctxp->iter_vars[i]) + return true; + error_at (loc, "variable %qE is not an iteration variable", var); + return false;I believe what we want to verify is that ith variable in the OMP_CLAUSE_DECL vector is iter_vars[i], so not just some random permutation etc.
Fixed.
@@ -3216,7 +3218,51 @@ check_omp_nesting_restrictions (gimple stmt, omp_context *ctx) break; } break; + case GIMPLE_OMP_TASK: + for (c = gimple_omp_task_clauses (stmt); c; c = OMP_CLAUSE_CHAIN (c)) + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND + && (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SOURCE + || OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)) + { + error_at (OMP_CLAUSE_LOCATION (c), + "depend(%s) is only available in 'omp ordered'",Please avoid using ' in diagnostics, it should be %<omp ordered%> instead.
Fixed.
+ OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SOURCE + ? "source" : "sink"); + return false; + } + break;This will eventually be needed also for GIMPLE_OMP_TARGET and GIMPLE_OMP_ENTER/EXIT_DATA. But as that isn't really supported right now, can wait.
I added an assert so we don't forget.
case GIMPLE_OMP_ORDERED: + for (c = gimple_omp_ordered_clauses (as_a <gomp_ordered *> (stmt)); + c; c = OMP_CLAUSE_CHAIN (c)) + { + enum omp_clause_depend_kind kind = OMP_CLAUSE_DEPEND_KIND (c); + if (kind == OMP_CLAUSE_DEPEND_SOURCE + || kind == OMP_CLAUSE_DEPEND_SINK) + { + bool have_ordered = false; + /* Look for containing ordered(N) loop. */ + for (omp_context *ctx_ = ctx; ctx_; ctx_ = ctx_->outer)Please use octx or something similar, I don't like the trailing _ ;)
I hate it too, but check_omp_nesting_restrictions() already had a use of ctx_ so I followed suit. Fixed in my code nevertheless.
+ if (!have_ordered) + { + error_at (OMP_CLAUSE_LOCATION (c), + "depend clause is not within an ordered loop");Not within is not the right OpenMP term, the requirement is that it must be closely nested in ordered loop.
Done.
+/* depend(sink...) is allowed without an offset. */ +#pragma omp ordered depend(sink:i,j+1)Can you write depend(sink:i,j-1) at least? The iteration to depend on must be lexicographically earlier in the loop.
Sure. Neither j+99 or j-HUGE are checked. We allow anything INTEGER_CST. Perhaps at expansion we can check the sanity of this? (Later, when we figure out what we're going to emit for the runtime).
+#pragma omp ordered depend(sink:i+2,j-2,k+2) /* { dg-error "is not an iteration var" } */Similarly. i-2 will be enough.--- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -1479,7 +1479,7 @@ remap_gimple_stmt (gimple stmt, copy_body_data *id) case GIMPLE_OMP_ORDERED: s1 = remap_gimple_seq (gimple_omp_body (stmt), id); - copy = gimple_build_omp_ordered (s1); + copy = gimple_build_omp_ordered (s1, NULL);You surely don't want to pass NULL here, I bet you want gimple_omp_ordered_clauses (stmt) instead.
Fixed.
--- a/gcc/tree-pretty-print.c +++ b/gcc/tree-pretty-print.c @@ -533,6 +533,9 @@ dump_omp_clause (pretty_printer *pp, tree clause, int spc, int flags) case OMP_CLAUSE_DEPEND_SOURCE: pp_string (pp, "source)"); return; + case OMP_CLAUSE_DEPEND_SINK: + pp_string (pp, "sink"); + break;And here you surely don't want to emit #pragma omp ordered(sink (note even the missing closing paren). It should dump the TREE_LIST (the var and if non-0 addend, the addend after it).
Notice this case had a break, not a return, so we would fall down to code that printed the TREE_LIST and added a closing parenthesis. The TREE_LIST was in the form of "i 3", which I thought was obvious enough. Be that as it may, I have added code to beautify it as "i+3" as suggested.
OK for branch?
Attachment:
curr
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |