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.129.124]) by sourceware.org (Postfix) with ESMTPS id EFF503858415 for ; Tue, 4 Oct 2022 12:58:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EFF503858415 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=1664888306; 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=uNLz5qU4nAkVUIlnj0KZ5VgLGtFCDOYmn2tYSPGqybg=; b=SEwQ7m6jLJ6PRJ4KMMHu1z0I7ka4qaWofoYx832mWXieCf1TdPH13G+HzgVxRBsqZSFN0s F8gQRdYvV5iT1FhXJf+1Qhn/amjOVrJGpp6lpm/Geiv4YJyA9zcvxHSFiEerWYMnF0c8Vx b/bFdAgjGHBG6oAX4zyR9w3bq+hrRXU= 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-127-0dBwKeqEOiCBEpZmY6g9Zw-1; Tue, 04 Oct 2022 08:58:23 -0400 X-MC-Unique: 0dBwKeqEOiCBEpZmY6g9Zw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 56E5C87B2A4; Tue, 4 Oct 2022 12:58:23 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.194]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 138807AE5; Tue, 4 Oct 2022 12:58:22 +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 294CwKXP3990965 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 4 Oct 2022 14:58:20 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 294CwJnc3990964; Tue, 4 Oct 2022 14:58:19 +0200 Date: Tue, 4 Oct 2022 14:58:18 +0200 From: Jakub Jelinek To: Tobias Burnus Cc: gcc-patches , fortran Subject: Re: [Patch] Fortran: Add OpenMP's assume(s) directives Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 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=-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 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 Tue, Oct 04, 2022 at 02:26:13PM +0200, Tobias Burnus wrote: > Hi Jakub, > > On 04.10.22 12:19, Jakub Jelinek wrote: > > On Sun, Oct 02, 2022 at 07:47:18PM +0200, Tobias Burnus wrote: > > > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -2749,9 +2749,9 @@ have support for @option{-pthread}. @option{-fopenmp} implies > @opindex fopenmp-simd > @cindex OpenMP SIMD > @cindex SIMD > -Enable handling of OpenMP's SIMD directives with @code{#pragma omp} > -in C/C++ and @code{!$omp} in Fortran. Other OpenMP directives > -are ignored. > +Enable handling of OpenMP's SIMD directives and OPENMP's @code{assume} directive > > > s/OPENMP/OpenMP/ > > We actually handle more directives, @code{declare reduction}, > @code{ordered}, @code{scan}, @code{loop} and combined/composite directives > with @code{simd} as constituent. > ... > And now in C++ we handle also the attribute syntax (guess we should update > the text for that here as well as in -fopenmp entry). > > Updated suggestion attached – I still need to update the main patch. > > (I also added 'declare simd' to the list. And I updated Fortran for scan + loop.) > > OK? Ok, thanks. > Wouldn't this be better table driven (like c_omp_directives > in c-family, though guess for Fortran you can just use spaces > in the name, don't need 3 strings for the separate tokens)? > Because I think absent/contains isn't the only spot where > you need directive names, metadirective is another. > > Maybe – I think there are already way to many string repetitions. One problem is that metadirectives permit combined/composite constructs while 'assume(s)' does not. I on purpose did not parse them, but probably in light of Metadirectives, I should. > > I will take a look. It is true that metadirective supports combined/composite constructs, and so do we in the C++ attribute case, still we IMHO can use the C/C++ table as is.and it doesn't need to include combined/composite constructs. The thing is that for the metadirective/C++ attribute case, all we need to know is to discover the directive category (declarative, stand-alone, construct, informational, ...) and for that it is enough to parse the first directive-name in combined/composite constructs. Both metadirectives and C++ attributes then have the name of the directive followed by clauses so we effectively have to use the normal parsing of directives/clauses there (except perhaps not end on end of directive but on unbalanced closing paren). And then there is the absent/contains case, where we only allow non-combined/composite, so there we need to try to match the directive name from the table and make sure it is followed by , or ). > But also, resizing each time a single entry is added to the list isn't > good for compile time, would be nice to grow the allocation size in > powers of 2 or so. > > I only expect a very small number – and did not want to keep track of yet another number. > > However, the following should work: > > > if (old_n_absent = 0) > absent = ... sizeof() * 1 > else if (popcount (old_n_absent) == 1) > absent = ... sizeof() * (old_n_absent) * 2) Yeah. Or for 0 allocate say 8 and use (pow2p_hwi (old_n_absent) && old_n_absent >= 8) in the else if. > that allocates: 1, 2, 4, 8, 16, 32, ... without keeping track of the number. > > > > +gfc_match_omp_assumes (void) > +{ > + locus loc = gfc_current_locus; > + gfc_omp_clauses *c = gfc_get_omp_clauses (); > + c->assume = gfc_current_ns->omp_assumes; > + if (!gfc_current_ns->proc_name > + || (gfc_current_ns->proc_name->attr.flavor != FL_MODULE > + && !gfc_current_ns->proc_name->attr.subroutine > + && !gfc_current_ns->proc_name->attr.function)) > + { > + gfc_error ("!OMP ASSUMES at %C must be in the specification part of a " > + "subprogram or module"); > + return MATCH_ERROR; > + } > + if (gfc_match_omp_clauses (&c, omp_mask (OMP_CLAUSE_ASSUMPTIONS), true, true, > + false, false, false, false) != MATCH_YES) > + { > + gfc_current_ns->omp_assumes = NULL; > + return MATCH_ERROR; > + } > > > > I don't understand the point of preallocation of gfc_omp_clauses here, > can't it be allocated inside of gfc_match_omp_clauses like everywhere else? > Because otherwise it e.g. leaks if the first error is reported. > > This is supposed to handle: > subroutine foo() > !$omp assumes absent(target) > !$omp assumes absent(teams) > end > > I did not spot anything which states that it is invalid. > (I might have missed it, however.) And if it is valid, > I assume it is equivalent to: > > subroutine foo() > !$omp assumes absent(target, teams) > end It is not equivalent to that, because while we have the restriction that the same list item can't appear multiple times on the same directive, it can appear multiple times on multiple directives. So, subroutine foo() !$omp assumes absent(target, target) end or subroutine foo() !$omp assumes absent(target) absent(target) end etc. are invalid but subroutine foo() !$omp assumes absent(target) !$omp assumes absent(target) end is fine (sure, redundant). > + for (int i = 0; i < assume->n_contains; i++) > + for (int j = i + 1; j < assume->n_contains; j++) > + if (assume->contains[i] == assume->contains[j]) > + gfc_error ("%qs directive mentioned multiple times in %s clause in %s " > + "directive at %L", > + gfc_ascii_statement (assume->contains[i], true), > + "CONTAINS", directive, loc); > > > > This is O(n^2)? Can't you use a bitmap or hash map instead? > > How about adding a 'break; after the all the gfc_error instead? True, I guess I can live with that. It is less user-friendly because it will print just one of the errors rather than all of them, though typically one will not have too many repetitions in there and can fix them one at a time as well. Jakub