From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26934 invoked by alias); 9 Jul 2015 18:53:24 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 26925 invoked by uid 89); 9 Jul 2015 18:53:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.2 required=5.0 tests=AWL,BAYES_50,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 09 Jul 2015 18:53:22 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 227E94750BC for ; Thu, 9 Jul 2015 18:53:21 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-116-82.ams2.redhat.com [10.36.116.82]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t69IrJZW013018 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 9 Jul 2015 14:53:20 -0400 Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.14.9/8.14.9) with ESMTP id t69IrG1Y014941; Thu, 9 Jul 2015 20:53:16 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.14.9/8.14.9/Submit) id t69IrFCj014391; Thu, 9 Jul 2015 20:53:15 +0200 Date: Thu, 09 Jul 2015 18:53:00 -0000 From: Jakub Jelinek To: Aldy Hernandez Cc: gcc-patches Subject: Re: [gomp4.1] depend(sink) and depend(source) parsing for C Message-ID: <20150709185315.GY10247@tucnak.redhat.com> Reply-To: Jakub Jelinek References: <559EBC6C.70109@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <559EBC6C.70109@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg00807.txt.bz2 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 % offset"); possible overflow looks weird. Shouldn't it complain the same as it does if you do: int c = - (-2147483648); ? > --- 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. > 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. > 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, ...). > @@ -149,6 +149,9 @@ struct gimplify_omp_ctx > struct gimplify_omp_ctx *outer_context; > splay_tree variables; > hash_set *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 instead. Then the size would be there as vec_length. > @@ -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. > @@ -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 % instead. > + 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. > case GIMPLE_OMP_ORDERED: > + for (c = gimple_omp_ordered_clauses (as_a (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 _ ;) > + 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. > +/* 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. > +#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. > --- 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). Jakub