From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 1674B393BC2C for ; Wed, 28 Apr 2021 13:41:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1674B393BC2C Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-256-VwtRfUzcMQiCoV74WsT8ag-1; Wed, 28 Apr 2021 09:41:48 -0400 X-MC-Unique: VwtRfUzcMQiCoV74WsT8ag-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EBFF09F92D; Wed, 28 Apr 2021 13:41:47 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-114-59.ams2.redhat.com [10.36.114.59]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5EFD05D9C6; Wed, 28 Apr 2021 13:41:46 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 13SDfhYs1744667 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 28 Apr 2021 15:41:44 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 13SDfhpJ1744666; Wed, 28 Apr 2021 15:41:43 +0200 Date: Wed, 28 Apr 2021 15:41:43 +0200 From: Jakub Jelinek To: Tobias Burnus Cc: gcc-patches , fortran Subject: Re: [Patch] OpenMP: Add iterator support to Fortran's depend; add affinity clause Message-ID: <20210428134143.GP1179226@tucnak> Reply-To: Jakub Jelinek References: <923778bf-f61e-7bd0-7926-53d28434fdab@codesourcery.com> MIME-Version: 1.0 In-Reply-To: <923778bf-f61e-7bd0-7926-53d28434fdab@codesourcery.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: fortran@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Fortran mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Apr 2021 13:41:53 -0000 On Tue, Apr 27, 2021 at 03:36:38PM +0200, Tobias Burnus wrote: > OpenMP 5's iterator can be used for > - depend clause > - affinity clause > - mapping (unsupported and not touched) > > (a) This patch add the iterator support to the Fortran FE > and adds support for it to the depend clause. > > (b) It also adds a conforming stub implementation (parse & ignore in ME) > for 'affinity' (Fortran, C, C++) > > (c) Fortran's taskwait did not handle the depend clause, now it does. > > The way the iterator is stored in Fortran is a bit convoluted, > but should be fine: > new namespace (such that symbols can be overridden and resolution works), > using symbol->value for the iteration (begin:end:step) as array constructor, > and abusing the ns->proc_name + its '->tlink' to generate an linked list, > which avoids walking the ns->sym_root tree and has the user's order in > the dump instead of the tree-walking order. > > The ME implementation also seems to require a very special way the > variables are stored. – It seems as if it works correctly for depend; > hence, I hope I did correctly read the dump and tree sharing is correctly > handled. > > NOTE: The iterator Fortran patch includes one change from the post-OpenMP-5.0 spec: > The '::' after the typespec in order to avoid Fortran free-form source issues with: Is the :: optional or required in free-form? > 'iterator(integer i=1:10)' – namely: is this the integer 'i' or the variable > 'integeri' (as spaces are ignored in fixed-form Fortran)? > NOTE 2: The way it is implemented, the 'begin:end:step' expression is evaluated > multiple times - once per list item; I think this matches C, but I am not completely > sure nor whether that is a problem. (Unlikely a real-world problem.) I believe C evaluates them just once and it would be nice if that could be the case for Fortran too. At least, looking at: int bar (int); void foo (void) { int a[64]; #pragma omp task depend (iterator (j=bar(0):bar(1):bar(2)) , out : a[j]) ; } I see all the bar calls just once: saved_stack.6 = __builtin_stack_save (); try { _1 = bar (0); _2 = bar (1); D.2078 = bar (2); and never afterwards. So, it would be nice if Fortran could do the same, wrap stuff in SAVE_EXPR or whatever is needed for that. And even void qux (void) { int a[64], b[64], c[64]; #pragma omp task depend (iterator (j=bar(0):bar(1):bar(2)) , out : a[j], b[j], c[j]) ; } seems to evaluate just once. > NOTE 3: I did have some trouble reading the spec with regards to what in C is permitted > for 'affinity' ('locator-list); the code now mostly follows what is permitted for 'depend'. locator-list is used for both affinity and depend, so I guess the same. I think at least for the latter C/C++ arranges that by using the same iterator tree for the adjacent vars, so that the middle-end knows it can just evaluate it once and emit one loop instead of one loop for each variable separately. Of course, depend (iterator (...),out : a[j]) depend(iterator (...),out : b[j]) will emit two different loops. > @@ -15508,6 +15511,52 @@ c_parser_omp_iterators (c_parser *parser) > return ret ? ret : error_mark_node; > } > > +/* OpenMP 5.0: > + affinity( [depend-modifier :] variable-list) For consistency, I think the syntax for other clauses puts space before ( and before ) too. Also, s/depend-modifier/aff-modifier/ > + depend-modifier: Likewise. > + iterator ( iterators-definition ) */ > + > +static tree > +c_parser_omp_clause_affinity (c_parser *parser, tree list) > +{ > + location_t clause_loc = c_parser_peek_token (parser)->location; > + tree nl, iterators = NULL_TREE; > + > + matching_parens parens; > + if (!parens.require_open (parser)) > + return list; > + > + if (c_parser_next_token_is (parser, CPP_NAME)) > + { > + const char *p = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value); > + if (strcmp ("iterator", p) == 0) > + { > + iterators = c_parser_omp_iterators (parser); > + if (!c_parser_require (parser, CPP_COLON, "expected %<:%>")) > + return list; I think in this case it should if (iterators) pop_scope (); parens.skip_until_found_close (parser); before doing return list; > + } > + } > + nl = c_parser_omp_variable_list (parser, clause_loc, OMP_CLAUSE_AFFINITY, > + list); > + if (iterators) > + { > + tree block = pop_scope (); > + if (iterators == error_mark_node) > + iterators = NULL_TREE; > + else > + { > + TREE_VEC_ELT (iterators, 5) = block; > + for (tree c = nl; c != list; c = OMP_CLAUSE_CHAIN (c)) > + OMP_CLAUSE_DECL (c) = build_tree_list (iterators, > + OMP_CLAUSE_DECL (c)); > + } > + } > + > + parens.skip_until_found_close (parser); > + return nl; > +} > + > + > @@ -14578,17 +14592,24 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > { > error_at (OMP_CLAUSE_LOCATION (c), > "%qE is not lvalue expression nor array section in " > - "% clause", t); > + "%qs clause", t, > + OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND > + ? "depend" : "affinity"); You could just use omp_clause_code_name[OMP_CLAUSE_CODE (c)] here. > remove = true; > } > else if (TREE_CODE (t) == COMPONENT_REF > && DECL_C_BIT_FIELD (TREE_OPERAND (t, 1))) > { > + gcc_assert (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND > + || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_AFFINITY); > error_at (OMP_CLAUSE_LOCATION (c), > - "bit-field %qE in %qs clause", t, "depend"); > + "bit-field %qE in %qs clause", t, > + OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND > + ? "depend" : "affinity"); Likewise. > @@ -37707,6 +37710,56 @@ cp_parser_omp_iterators (cp_parser *parser) > return ret ? ret : error_mark_node; > } > > +/* OpenMP 5.0: > + affinity( [depend-modifier :] variable-list) > + depend-modifier: > + iterator ( iterators-definition ) */ See above. > + > +static tree > +cp_parser_omp_clause_affinity (cp_parser *parser, tree list) > +{ > + tree nlist, c, iterators = NULL_TREE; > + > + matching_parens parens; > + if (!parens.require_open (parser)) > + return list; > + > + if (cp_lexer_next_token_is (parser->lexer, CPP_NAME)) > + { > + tree id = cp_lexer_peek_token (parser->lexer)->u.value; > + const char *p = IDENTIFIER_POINTER (id); > + if (strcmp ("iterator", p) == 0) > + { > + begin_scope (sk_omp, NULL); > + iterators = cp_parser_omp_iterators (parser); > + if (!cp_parser_require (parser, CPP_COLON, RT_COLON)) > + { Again, missing if (iterators) poplevel (0, 1, 0); here. > + cp_parser_skip_to_closing_parenthesis (parser, > + /*recovering=*/true, > + /*or_comma=*/false, > + /*consume_paren=*/true); > + return list; > + } > + } > + } > @@ -7461,7 +7471,9 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > { > if (handle_omp_array_sections (c, ort)) > remove = true; > - else if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_DEPOBJ) > + else if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND > + && OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_DEPOBJ) I think ()s should be added (I think at least emacs prefers that) when the && rhs doesn't fit on one line. > @@ -7486,22 +7498,31 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > if (DECL_P (t)) > error_at (OMP_CLAUSE_LOCATION (c), > "%qD is not lvalue expression nor array section " > - "in % clause", t); > + "in %qs clause", t, > + OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND > + ? "depend" : "affinity"); > else > error_at (OMP_CLAUSE_LOCATION (c), > "%qE is not lvalue expression nor array section " > - "in % clause", t); > + "in %qs clause", t, > + OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND > + ? "depend" : "affinity"); See above 2x. > remove = true; > } > else if (TREE_CODE (t) == COMPONENT_REF > && TREE_CODE (TREE_OPERAND (t, 1)) == FIELD_DECL > && DECL_BIT_FIELD (TREE_OPERAND (t, 1))) > { > + gcc_assert (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND > + || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_AFFINITY); > error_at (OMP_CLAUSE_LOCATION (c), > - "bit-field %qE in %qs clause", t, "depend"); > + "bit-field %qE in %qs clause", t, > + OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND > + ? "depend" : "affinity"); And again. > @@ -261,6 +263,7 @@ gfc_match_omp_variable_list (const char *str, gfc_omp_namelist **list, > case MATCH_YES: > gfc_expr *expr; > expr = NULL; > + gfc_gobble_whitespace (); > if ((allow_sections && gfc_peek_ascii_char () == '(') > || (allow_derived && gfc_peek_ascii_char () == '%')) > { Is this change specific to depend/affinity or iterators? If not, shouldn't it go in separately and with a testcase that shows when it is needed? > --- a/gcc/gimplify.c > +++ b/gcc/gimplify.c > @@ -9506,6 +9506,10 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p, > > goto do_add; > > + case OMP_CLAUSE_AFFINITY: > + /* Ignore. */ > + remove = true; > + break; > case OMP_CLAUSE_DEPEND: > if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK) > { I think removing affinity at this point is probably too early. That will mean we e.g. won't diagnose invalid programs that e.g. use default(none) on some outer OpenMP construct and then refer to variables that aren't mentioned in any of the parallel/host teams clauses with such default(none). We don't need to emit the loops we emit for depend, sure (though if it is easier we could and hope for it to be optimized away). Or at least we should evaluate the iterator expressions for side-effects (i.e. gimplify them and ignore) and probably gimplify the affinity vars too. If we emit the loops, just gimplify the location-list items in the loop and then throw them away, if not, evaluate it like step; iter = beg; if (iter cond end) expr; or so (so that expr isn't evaluated if the cond isn't true and all of beg, end and step are evaluated. > diff --git a/gcc/testsuite/c-c++-common/gomp/affinity-1.c b/gcc/testsuite/c-c++-common/gomp/affinity-1.c > new file mode 100644 > index 00000000000..558e316bccd > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/gomp/affinity-1.c > @@ -0,0 +1,20 @@ > +void > +foo(int x) > +{ > + int a, b[5], cc, d[5][5]; Can you please initialize the vars (at least those that are actually used like a)? > +#pragma omp taskgroup > + { > + #pragma omp task affinity(a) > + { } > + #pragma omp task affinity(iterator(i=(int)__builtin_cos(1.0+a):5, jj =2:5:2) : b[i], d[i][jj]) > + { } > + #pragma omp task affinity(iterator(i=(int)__builtin_cos(1.0+a):5) : b[i], d[i][i]) > + { } > + #pragma omp task affinity (iterator(i=1:5): a) > + { } > + #pragma omp task affinity (iterator(i=1:5): a) affinity(iterator(i=1:5) : x) > + { } > + #pragma omp task affinity (iterator(unsigned long j=1:5, k=7:4:-1) : b[j+k],a) affinity (cc) > + { } Perhaps just ; instead of { }, your choice. Jakub