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 B2D18382E292 for ; Tue, 24 May 2022 13:03:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B2D18382E292 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-561-TW1Qw7b8MGSW6z5hGBPrvg-1; Tue, 24 May 2022 09:03:14 -0400 X-MC-Unique: TW1Qw7b8MGSW6z5hGBPrvg-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C417018812C7; Tue, 24 May 2022 13:03:11 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.106]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7B952492C3B; Tue, 24 May 2022 13:03:11 +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 24OD38Gp2106960 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 24 May 2022 15:03:08 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 24OD37NN2106959; Tue, 24 May 2022 15:03:07 +0200 Date: Tue, 24 May 2022 15:03:07 +0200 From: Jakub Jelinek To: Julian Brown Cc: gcc-patches@gcc.gnu.org, Thomas Schwinge , Tobias Burnus , Fortran List Subject: Re: [PATCH v2 01/11] OpenMP 5.0: Clause ordering for OpenMP 5.0 (topological sorting by base pointer) Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.85 on 10.11.54.9 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=-10.2 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, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Tue, 24 May 2022 13:03:32 -0000 On Fri, Mar 18, 2022 at 09:24:51AM -0700, Julian Brown wrote: > 2021-11-23 Julian Brown > > gcc/ > * gimplify.c (is_or_contains_p, omp_target_reorder_clauses): Delete > functions. > (omp_tsort_mark): Add enum. > (omp_mapping_group): Add struct. > (debug_mapping_group, omp_get_base_pointer, omp_get_attachment, > omp_group_last, omp_gather_mapping_groups, omp_group_base, > omp_index_mapping_groups, omp_containing_struct, > omp_tsort_mapping_groups_1, omp_tsort_mapping_groups, > omp_segregate_mapping_groups, omp_reorder_mapping_groups): New > functions. > (gimplify_scan_omp_clauses): Call above functions instead of > omp_target_reorder_clauses, unless we've seen an error. > * omp-low.c (scan_sharing_clauses): Avoid strict test if we haven't > sorted mapping groups. > > gcc/testsuite/ > * g++.dg/gomp/target-lambda-1.C: Adjust expected output. > * g++.dg/gomp/target-this-3.C: Likewise. > * g++.dg/gomp/target-this-4.C: Likewise. > + Wouldn't hurt to add a comment on the meanings of the enumerators. > +enum omp_tsort_mark { > + UNVISITED, > + TEMPORARY, > + PERMANENT > +}; > + > +struct omp_mapping_group { > + tree *grp_start; > + tree grp_end; > + omp_tsort_mark mark; > + struct omp_mapping_group *sibling; > + struct omp_mapping_group *next; > +}; > + > +__attribute__((used)) static void I'd use what is used elsewhere, DEBUG_FUNCTION void without static. > +debug_mapping_group (omp_mapping_group *grp) > +{ > + tree tmp = OMP_CLAUSE_CHAIN (grp->grp_end); > + OMP_CLAUSE_CHAIN (grp->grp_end) = NULL; > + debug_generic_expr (*grp->grp_start); > + OMP_CLAUSE_CHAIN (grp->grp_end) = tmp; > +} > + > +/* Return the OpenMP "base pointer" of an expression EXPR, or NULL if there > + isn't one. This needs improvement. */ > + > +static tree > +omp_get_base_pointer (tree expr) > +{ > + while (TREE_CODE (expr) == ARRAY_REF) > + expr = TREE_OPERAND (expr, 0); > + > + while (TREE_CODE (expr) == COMPONENT_REF > + && (DECL_P (TREE_OPERAND (expr, 0)) > + || (TREE_CODE (TREE_OPERAND (expr, 0)) == COMPONENT_REF) > + || TREE_CODE (TREE_OPERAND (expr, 0)) == INDIRECT_REF > + || (TREE_CODE (TREE_OPERAND (expr, 0)) == MEM_REF > + && integer_zerop (TREE_OPERAND (TREE_OPERAND (expr, 0), 1))) > + || TREE_CODE (TREE_OPERAND (expr, 0)) == ARRAY_REF)) > + { > + expr = TREE_OPERAND (expr, 0); > + > + while (TREE_CODE (expr) == ARRAY_REF) > + expr = TREE_OPERAND (expr, 0); > + > + if (TREE_CODE (expr) == INDIRECT_REF || TREE_CODE (expr) == MEM_REF) > + break; > + } I must say I don't see advantages of just a single loop that looks through all ARRAY_REFs and all COMPONENT_REFs and then just checks if the expr it got in the end is a decl or INDIRECT_REF or MEM_REF with offset 0. > + if (DECL_P (expr)) > + return NULL_TREE; > + > + if (TREE_CODE (expr) == INDIRECT_REF > + || TREE_CODE (expr) == MEM_REF) > + { > + expr = TREE_OPERAND (expr, 0); > + while (TREE_CODE (expr) == COMPOUND_EXPR) > + expr = TREE_OPERAND (expr, 1); > + if (TREE_CODE (expr) == POINTER_PLUS_EXPR) > + expr = TREE_OPERAND (expr, 0); > + if (TREE_CODE (expr) == SAVE_EXPR) > + expr = TREE_OPERAND (expr, 0); > + STRIP_NOPS (expr); > + return expr; > + } > + > + return NULL_TREE; > +} > + > +static tree > +omp_containing_struct (tree expr) > +{ > + tree expr0 = expr; > + > + STRIP_NOPS (expr); > + > + tree expr1 = expr; > + > + /* FIXME: other types of accessors? */ > + while (TREE_CODE (expr) == ARRAY_REF) > + expr = TREE_OPERAND (expr, 0); > + > + if (TREE_CODE (expr) == COMPONENT_REF) > + { > + if (DECL_P (TREE_OPERAND (expr, 0)) > + || TREE_CODE (TREE_OPERAND (expr, 0)) == COMPONENT_REF > + || TREE_CODE (TREE_OPERAND (expr, 0)) == INDIRECT_REF > + || (TREE_CODE (TREE_OPERAND (expr, 0)) == MEM_REF > + && integer_zerop (TREE_OPERAND (TREE_OPERAND (expr, 0), 1))) > + || TREE_CODE (TREE_OPERAND (expr, 0)) == ARRAY_REF) > + expr = TREE_OPERAND (expr, 0); > + else > + internal_error ("unhandled component"); > + } Again? > @@ -9063,11 +9820,29 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p, > break; > } > > - if (code == OMP_TARGET > - || code == OMP_TARGET_DATA > - || code == OMP_TARGET_ENTER_DATA > - || code == OMP_TARGET_EXIT_DATA) > - omp_target_reorder_clauses (list_p); > + /* Topological sorting may fail if we have duplicate nodes, which > + we should have detected and shown an error for already. Skip > + sorting in that case. */ > + if (!seen_error () > + && (code == OMP_TARGET > + || code == OMP_TARGET_DATA > + || code == OMP_TARGET_ENTER_DATA > + || code == OMP_TARGET_EXIT_DATA)) > + { > + vec *groups; > + groups = omp_gather_mapping_groups (list_p); > + if (groups) > + { > + hash_map *grpmap; > + grpmap = omp_index_mapping_groups (groups); > + omp_mapping_group *outlist > + = omp_tsort_mapping_groups (groups, grpmap); > + outlist = omp_segregate_mapping_groups (outlist); > + list_p = omp_reorder_mapping_groups (groups, outlist, list_p); > + delete grpmap; > + delete groups; > + } > + } I think big question is if we do want to do this map clause reordering before processing the omp target etc. clauses, or after (during gimplify_adjust_omp_clauses, when clauses from the implicit mappings are added too and especially with the declare mapper expansions), or both before and after. > while ((c = *list_p) != NULL) > { > diff --git a/gcc/omp-low.cc b/gcc/omp-low.cc > index c33b3daa439..ffeb1f34fd7 100644 > --- a/gcc/omp-low.cc > +++ b/gcc/omp-low.cc > @@ -1537,8 +1537,11 @@ scan_sharing_clauses (tree clauses, omp_context *ctx) > { > /* If this is an offloaded region, an attach operation should > only exist when the pointer variable is mapped in a prior > - clause. */ > - if (is_gimple_omp_offloaded (ctx->stmt)) > + clause. > + If we had an error, we may not have attempted to sort clauses > + properly, so avoid the test. */ > + if (is_gimple_omp_offloaded (ctx->stmt) > + && !seen_error ()) If we encounter a major error during processing map clauses, we should consider just leaving out the offloading construct from the IL. Jakub