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 ESMTPS id 375933857C67 for ; Thu, 8 Sep 2022 15:21:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 375933857C67 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1662650487; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=572weFme3x0Z1hePnll0yI3A5CpdY5V7Sc626uYXnvg=; b=g2T0yst0SlxRTx2lAKpEOUWVxodhNJw8u8+mTyPwUM39QsLv4LKfnTvN3JbMbR8qpjKN2u El7lTMp+VwcOigTDLvblOYFNknD7HHw2yk8KCDOXxC41uaZM5mCYciNDCusuWkLSBDzKxk 7JQ9mMt4xeftlMviIUWFKtRlEvJYolk= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-528-zR_fDtMkNOW_wM4eiOTwkw-1; Thu, 08 Sep 2022 11:21:24 -0400 X-MC-Unique: zR_fDtMkNOW_wM4eiOTwkw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9AA7185A58A; Thu, 8 Sep 2022 15:21:23 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 56434403D0DA; Thu, 8 Sep 2022 15:21:23 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 288FLAtq1563711 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 8 Sep 2022 17:21:15 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 288FL8oG1563710; Thu, 8 Sep 2022 17:21:08 +0200 Date: Thu, 8 Sep 2022 17:21:08 +0200 From: Jakub Jelinek To: Tobias Burnus Cc: gcc-patches , fortran Subject: Re: [Patch] OpenMP/Fortran: Permit end-clause on directive Message-ID: Reply-To: Jakub Jelinek References: <821786f3-ac7b-01e3-a386-f7c082494022@codesourcery.com> MIME-Version: 1.0 In-Reply-To: <821786f3-ac7b-01e3-a386-f7c082494022@codesourcery.com> X-Scanned-By: MIMEDefang 2.84 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=WINDOWS-1252 Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, Aug 26, 2022 at 08:21:26PM +0200, Tobias Burnus wrote: > I did run into some issues related to this; those turned out to be > unrelated, but I end ended up implementing this feature. > > Side remark: 'omp parallel workshare' seems to actually permit 'nowait' > now, but I guess that's an unintended change due to the > syntax-representation change. Hence, it is now tracked as Spec Issue > 3338 and I do not permit it. > > OK for mainline? > > Tobias > ----------------- > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 > OpenMP/Fortran: Permit end-clause on directive > > gcc/fortran/ChangeLog: > > * openmp.cc (OMP_DO_CLAUSES, OMP_SCOPE_CLAUSES, > OMP_SECTIONS_CLAUSES, OMP_SINGLE_CLAUSES): Add 'nowait'. This doesn't describe what the patch actually does, Add 'nowait'. is only true for the first 3, for OMP_SINGLE_CLAUSES IMHO you want a separate (OMP_SINGLE_CLAUSES): Add 'nowait' and 'copyprivate'. entry. > @@ -3855,7 +3857,7 @@ cleanup: > | OMP_CLAUSE_ORDER | OMP_CLAUSE_ALLOCATE) > #define OMP_SINGLE_CLAUSES \ > (omp_mask (OMP_CLAUSE_PRIVATE) | OMP_CLAUSE_FIRSTPRIVATE \ > - | OMP_CLAUSE_ALLOCATE) > + | OMP_CLAUSE_ALLOCATE | OMP_CLAUSE_NOWAIT | OMP_CLAUSE_COPYPRIVATE) > #define OMP_ORDERED_CLAUSES \ > (omp_mask (OMP_CLAUSE_THREADS) | OMP_CLAUSE_SIMD) > #define OMP_DECLARE_TARGET_CLAUSES \ > @@ -5909,13 +5915,11 @@ gfc_match_omp_teams_distribute_simd (void) > match > gfc_match_omp_workshare (void) > { > - if (gfc_match_omp_eos () != MATCH_YES) > - { > - gfc_error ("Unexpected junk after $OMP WORKSHARE statement at %C"); > - return MATCH_ERROR; > - } > + gfc_omp_clauses *c; > + if (gfc_match_omp_clauses (&c, omp_mask (OMP_CLAUSE_NOWAIT)) != MATCH_YES) > + return MATCH_ERROR; > new_st.op = EXEC_OMP_WORKSHARE; > - new_st.ext.omp_clauses = gfc_get_omp_clauses (); > + new_st.ext.omp_clauses = c; > return MATCH_YES; > } I think it would be better to introduce OMP_WORKSHARE_CLAUSES and use it in both gfc_match_omp_workshare and just use return match_omp (EXEC_OMP_WORKSHARE, OMP_WORKSHARE_CLAUSES); ? > @@ -6954,6 +6952,9 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses, > } > break; > case OMP_LIST_COPYPRIVATE: > + if (omp_clauses->nowait) > + gfc_error ("NOWAIT clause must not be be used with COPYPRIVATE " s/be be/be/ > + "clause at %L", &n->where); > for (; n != NULL; n = n->next) > { > if (n->sym->as && n->sym->as->type == AS_ASSUMED_SIZE) > @@ -5284,7 +5285,13 @@ parse_omp_do (gfc_statement omp_st) > if (st == omp_end_st) > { > if (new_st.op == EXEC_OMP_END_NOWAIT) > - cp->ext.omp_clauses->nowait |= new_st.ext.omp_bool; > + { > + if (cp->ext.omp_clauses->nowait && new_st.ext.omp_bool) > + gfc_error_now ("Duplicated NOWAIT clause on %s and %s at %C", > + gfc_ascii_statement (omp_st), > + gfc_ascii_statement (omp_end_st)); > + cp->ext.omp_clauses->nowait |= new_st.ext.omp_bool; > + } > else > gcc_assert (new_st.op == EXEC_NOP); > gfc_clear_new_st (); Not sure if the standard is clear enough that unique clauses can't be repeated on both directive and corresponding end directive. But let's assume that is the case. > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/gomp/copyprivate-2.f90 > @@ -0,0 +1,69 @@ > + FUNCTION t() > + INTEGER :: a, b, t > + a = 0 > + t = b > + b = 0 > + !$OMP PARALLEL REDUCTION(+:b) > + !$OMP SINGLE COPYPRIVATE (b) NOWAIT ! { dg-error "NOWAIT clause must not be be used with COPYPRIVATE clause" } Here too (several times). > + !$OMP ATOMIC WRITE > + b = 6 > + !$OMP END SINGLE > + !$OMP END PARALLEL > + t = t + b > + END FUNCTION > + > + FUNCTION t2() > + INTEGER :: a, b, t2 > + a = 0 > + t2 = b > + b = 0 > + !$OMP PARALLEL REDUCTION(+:b) > + !$OMP SINGLE NOWAIT COPYPRIVATE (b) ! { dg-error "NOWAIT clause must not be be used with COPYPRIVATE clause" } > + !$OMP ATOMIC WRITE > + b = 6 > + !$OMP END SINGLE > + !$OMP END PARALLEL > + t2 = t2 + b > + END FUNCTION > + > + FUNCTION t3() > + INTEGER :: a, b, t3 > + a = 0 > + t3 = b > + b = 0 > + !$OMP PARALLEL REDUCTION(+:b) > + !$OMP SINGLE COPYPRIVATE (b) ! { dg-error "NOWAIT clause must not be be used with COPYPRIVATE clause" } > + !$OMP ATOMIC WRITE > + b = 6 > + !$OMP END SINGLE NOWAIT > + !$OMP END PARALLEL > + t3 = t3 + b > + END FUNCTION > + > + FUNCTION t4() > + INTEGER :: a, b, t4 > + a = 0 > + t4 = b > + b = 0 > + !$OMP PARALLEL REDUCTION(+:b) > + !$OMP SINGLE > + !$OMP ATOMIC WRITE > + b = 6 > + !$OMP END SINGLE NOWAIT COPYPRIVATE (b) ! { dg-error "NOWAIT clause must not be be used with COPYPRIVATE clause" } > + !$OMP END PARALLEL > + t4 = t4 + b > + END FUNCTION > + > + FUNCTION t5() > + INTEGER :: a, b, t5 > + a = 0 > + t5 = b > + b = 0 > + !$OMP PARALLEL REDUCTION(+:b) > + !$OMP SINGLE > + !$OMP ATOMIC WRITE > + b = 6 > + !$OMP END SINGLE COPYPRIVATE (b) NOWAIT ! { dg-error "NOWAIT clause must not be be used with COPYPRIVATE clause" } > + !$OMP END PARALLEL > + t5 = t5 + b > + END FUNCTION I think this lacks a test for !$OMP SINGLE NOWAIT and !$OMP END SINGLE COPYPRIVATE (b). Also, shouldn't we have test coverage for !$OMP SINGLE COPYPRIVATE (b) with !$OMP END SINGLE COPYPRIVATE (b) (that we detect multiple copyprivate clauses for the same variable even that way)? Otherwise LGTM. Note, for combined constructs with target seems we were already implementing this, because the 5.1 wording allows nowait only on !$omp target and not on !$omp end target, right? Jakub