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 141CC3858401 for ; Tue, 11 Oct 2022 12:13:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 141CC3858401 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=1665490402; 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:in-reply-to:in-reply-to: references:references; bh=mO9lbdgbpcMORljsPJ5pdpgXH88nH5IICAfNQ8CeKLQ=; b=CDTVtsROmglfT0rMn8yIhmV3tnHVNeEK01hLtLWeBEXI3wIdt+Bxg8Bo7j05k0mqijfITX mLUoqi9z2+8f6ZejzzeifCuh1lQZamNkKgBLWAFFGHpD5K9cjzJWW2MKRDqkOUvgzGyOYO V5/HLxPGOweEAP+H9H3FX/EKk09yCRo= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-108-jeM9JljWOqqJs3NoBMP8Sw-1; Tue, 11 Oct 2022 08:13:19 -0400 X-MC-Unique: jeM9JljWOqqJs3NoBMP8Sw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 2D9BF1C1A946; Tue, 11 Oct 2022 12:13:19 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.55]) by smtp.corp.redhat.com (Postfix) with ESMTPS id DA6CC40C206B; Tue, 11 Oct 2022 12:13:18 +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 29BCDGjE2943100 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 11 Oct 2022 14:13:16 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 29BCDFKe2943099; Tue, 11 Oct 2022 14:13:15 +0200 Date: Tue, 11 Oct 2022 14:13:14 +0200 From: Jakub Jelinek To: Hafiz Abid Qadeer Cc: gcc-patches@gcc.gnu.org, fortran@gcc.gnu.org, tobias@codesourcery.com Subject: Re: [PATCH 1/5] [gfortran] Add parsing support for allocate directive (OpenMP 5.0). Message-ID: Reply-To: Jakub Jelinek References: <20220113145320.3153087-1-abidh@codesourcery.com> <20220113145320.3153087-2-abidh@codesourcery.com> MIME-Version: 1.0 In-Reply-To: <20220113145320.3153087-2-abidh@codesourcery.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham 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 Thu, Jan 13, 2022 at 02:53:16PM +0000, Hafiz Abid Qadeer wrote: > Currently we only make use of this directive when it is associated > with an allocate statement. Sorry for the delay. I'll start with a comment that allocate directive in 5.0/5.1 for Fortran is a complete mess that has been fixed only in 5.2 by splitting the directive into the allocators construct and allocate directive. The problem with 5.0/5.1 is that it is just ambiguous whether !$omp allocate (list) optional-clauses is associated with an allocate statement or not. When it is not associated with allocate statement, it is a declarative directive that should appear only in the specification part, when it is associated with a allocate stmt, it should appear only in the executable part. And a mess starts when it is on the boundary between the two. Now, how exactly to differentiate between the 2 I'm afraid depends on the exact OpenMP version. 1) if we are p->state == ORDER_EXEC already, it must be associated with allocate-stmt (and we should error whenever it violates restrictions for those) 2) if (list) is missing, it must be associated with allocate-stmt 3) for 5.0 only, if allocator clause isn't specified, it must be not associated with allocate-stmt, but in 5.1 the clauses are optional also for one associated with it; if align clause is specified, it must be 5.1 4) all the allocate directives after one that must be associated with allocate-stmt must be also associated with allocate-stmt 5) if variables in list are allocatable, it must be associated with allocate-stmt, if they aren't allocatable, it must not be associated with allocate-stmt In your patch, you put ST_OMP_ALLOCATE into case_executable define, I'm afraid due to the above we need to handle ST_OMP_ALLOCATE manually whenever case_executable/case_omp_decl appear in parse.cc and be prepared that it could be either declarative directive or executable construct and resolve based on the 1-5 above into which category it belongs (either during parsing or during resolving). And certainly have testsuite coverage for cases like: integer :: i, j integer, allocatable :: k(:), l(:) !$omp allocate (i) allocator (alloc1) !$omp allocate (j) allocator (alloc2) !$omp allocate (k) allocator (alloc3) !$omp allocate (l) allocator (alloc4) allocate (k(14), l(23)) where I think the first 2 are declarative directives and the last 2 bind to allocate-stmt (etc., cover all the cases mentioned above). On the other side, 5.1 has: "An allocate directive that is associated with an allocate-stmt and specifies a list must be preceded by an executable statement or OpenMP construct." restriction, so if we implement that, the ambiguity decreases. We wouldn't need to worry about 3) and 5), would decide on 1) and 2) and 4) only. > gcc/fortran/ChangeLog: > > * dump-parse-tree.c (show_omp_node): Handle EXEC_OMP_ALLOCATE. > (show_code_node): Likewise. > * gfortran.h (enum gfc_statement): Add ST_OMP_ALLOCATE. > (OMP_LIST_ALLOCATOR): New enum value. > (enum gfc_exec_op): Add EXEC_OMP_ALLOCATE. > * match.h (gfc_match_omp_allocate): New function. > * openmp.c (enum omp_mask1): Add OMP_CLAUSE_ALLOCATOR. > (OMP_ALLOCATE_CLAUSES): New define. > (gfc_match_omp_allocate): New function. > (resolve_omp_clauses): Add ALLOCATOR in clause_names. > (omp_code_to_statement): Handle EXEC_OMP_ALLOCATE. > (EMPTY_VAR_LIST): New define. > (check_allocate_directive_restrictions): New function. > (gfc_resolve_omp_allocate): Likewise. > (gfc_resolve_omp_directive): Handle EXEC_OMP_ALLOCATE. > * parse.c (decode_omp_directive): Handle ST_OMP_ALLOCATE. > (next_statement): Likewise. You didn't change next_statement, but case_executable macro. But see above. > (gfc_ascii_statement): Likewise. > * resolve.c (gfc_resolve_code): Handle EXEC_OMP_ALLOCATE. > * st.c (gfc_free_statement): Likewise. > * trans.c (trans_code): Likewise > > gcc/testsuite/ChangeLog: > > * gfortran.dg/gomp/allocate-4.f90: New test. > * gfortran.dg/gomp/allocate-5.f90: New test. > --- > --- a/gcc/fortran/openmp.c > +++ b/gcc/fortran/openmp.c > @@ -921,6 +921,7 @@ enum omp_mask1 > OMP_CLAUSE_FAIL, /* OpenMP 5.1. */ > OMP_CLAUSE_WEAK, /* OpenMP 5.1. */ > OMP_CLAUSE_NOWAIT, > + OMP_CLAUSE_ALLOCATOR, I don't see how can you add OMP_CLAUSE_ALLOCATOR to enum omp_mask1. OMP_MASK1_LAST is already 64, so I think the gcc_checking_assert (OMP_MASK1_LAST <= 64 && OMP_MASK2_LAST <= 64); assertion would fail. OMP_MASK2_LAST is on the other side just 30, and allocate directive takes just allocator or in 5.1 align clauses, so both should go to the enum omp_mask2 block. And for newly added clauses, we add the /* OpenMP 5.0. */ etc. comments when the clause appeared first (5.0 for allocator, 5.1 for align). > /* This must come last. */ > OMP_MASK1_LAST > }; > @@ -3568,6 +3569,7 @@ cleanup: > } > > > +#define OMP_ALLOCATE_CLAUSES (omp_mask (OMP_CLAUSE_ALLOCATOR)) You define the above. > #define OMP_PARALLEL_CLAUSES \ > (omp_mask (OMP_CLAUSE_PRIVATE) | OMP_CLAUSE_FIRSTPRIVATE \ > | OMP_CLAUSE_SHARED | OMP_CLAUSE_COPYIN | OMP_CLAUSE_REDUCTION \ > @@ -5762,6 +5764,64 @@ gfc_match_omp_ordered_depend (void) > return match_omp (EXEC_OMP_ORDERED, omp_mask (OMP_CLAUSE_DEPEND)); > } > > +/* omp allocate (list) [clause-list] > + - clause-list: allocator > +*/ > + > +match > +gfc_match_omp_allocate (void) > +{ > + gfc_omp_clauses *c = gfc_get_omp_clauses (); > + gfc_expr *allocator = NULL; > + match m; > + > + m = gfc_match (" ("); > + if (m == MATCH_YES) > + { > + m = gfc_match_omp_variable_list ("", &c->lists[OMP_LIST_ALLOCATOR], > + true, NULL); > + > + if (m != MATCH_YES) > + { > + /* If the list was empty, we must find closing ')'. */ Empty list should be invalid if ( is seen, no? > + m = gfc_match (")"); > + if (m != MATCH_YES) > + return m; > + } > + } > + > + if (gfc_match (" allocator ( ") == MATCH_YES) > + { > + m = gfc_match_expr (&allocator); > + if (m != MATCH_YES) > + { > + gfc_error ("Expected allocator at %C"); > + return MATCH_ERROR; > + } > + if (gfc_match (" ) ") != MATCH_YES) > + { > + gfc_error ("Expected ')' at %C"); > + gfc_free_expr (allocator); > + return MATCH_ERROR; > + } > + } But then parse the allocator clause by hand, so OMP_ALLOCATE_CLAUSES is never used. I think it would be better to go through the normal clause parsing because we'll need to handle align clause too soon and while there can be at most one allocator clause and at most one align clause, they can appear in either order, and there can or doesn't have to be a comma in between them. > + if (!omp_al || gfc_extract_int (omp_al, &tmp)) > + gfc_error ("%qs should use predefined allocator at %L", sym->name, > + &loc); > + } > + if (ns != sym->ns) > + gfc_error ("%qs is not in the same scope as %" > + " directive at %L", sym->name, &loc); > +} > + > +#define EMPTY_VAR_LIST(node) \ > + (node->ext.omp_clauses->lists[OMP_LIST_ALLOCATOR] == NULL) > + > +static void > +gfc_resolve_omp_allocate (gfc_code *code, gfc_namespace *ns) > +{ > + gfc_alloc *al; > + gfc_omp_namelist *n = NULL; > + gfc_omp_namelist *cn = NULL; > + gfc_omp_namelist *p, *tail; > + gfc_code *cur; > + hash_set vars; > + > + gfc_omp_clauses *clauses = code->ext.omp_clauses; > + gcc_assert (clauses); > + cn = clauses->lists[OMP_LIST_ALLOCATOR]; > + gfc_expr *omp_al = cn ? cn->expr : NULL; > + > + if (omp_al && (omp_al->ts.type != BT_INTEGER > + || omp_al->ts.kind != gfc_c_intptr_kind)) The formatting is weird, || should be below omp_al-> > + gfc_error ("Expected integer expression of the " > + "% kind at %L", &omp_al->where); Jakub