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 4FD54385DC33 for ; Thu, 24 Jun 2021 13:15:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4FD54385DC33 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-64-JijNCa_iNB6XJdA3QfTsXg-1; Thu, 24 Jun 2021 09:15:56 -0400 X-MC-Unique: JijNCa_iNB6XJdA3QfTsXg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8A1EC802C8A; Thu, 24 Jun 2021 13:15:55 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-112-143.ams2.redhat.com [10.36.112.143]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1D12CE2C4; Thu, 24 Jun 2021 13:15:54 +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 15ODFqvd1297117 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 24 Jun 2021 15:15:52 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 15ODFpix1297116; Thu, 24 Jun 2021 15:15:51 +0200 Date: Thu, 24 Jun 2021 15:15:51 +0200 From: Jakub Jelinek To: Chung-Lin Tang Cc: gcc-patches , Catherine Moore Subject: Re: [PATCH, OpenMP 5.0] Improve OpenMP target support for C++ [PR92120 v4] Message-ID: <20210624131551.GR7746@tucnak> Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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=-5.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Jun 2021 13:16:00 -0000 On Fri, Jun 18, 2021 at 10:25:16PM +0800, Chung-Lin Tang wrote: Note, you'll need to rebase your patch, it clashes with r12-1768-g7619d33471c10fe3d149dcbb701d99ed3dd23528. Sorry for that. And sorry for patch review delay. > --- a/gcc/c/c-typeck.c > +++ b/gcc/c/c-typeck.c > @@ -13104,6 +13104,12 @@ handle_omp_array_sections_1 (tree c, tree t, vec &types, > return error_mark_node; > } > t = TREE_OPERAND (t, 0); > + if ((ort == C_ORT_ACC || ort == C_ORT_OMP) Map clauses never appear on declare simd, so (ort == C_ORT_ACC || ort == C_ORT_OMP) previously meant always and since the in_reduction change is incorrect (as C_ORT_OMP_TARGET is used for target construct but not for e.g. target data* or target update). > + && TREE_CODE (t) == MEM_REF) So please just use if (TREE_CODE (t) == MEM_REF) or explain when it shouldn't trigger. > @@ -14736,6 +14743,11 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > { > while (TREE_CODE (t) == COMPONENT_REF) > t = TREE_OPERAND (t, 0); > + if (TREE_CODE (t) == MEM_REF) > + { > + t = TREE_OPERAND (t, 0); > + STRIP_NOPS (t); > + } This doesn't look correct. At least the parsing (and the spec AFAIK) doesn't ensure that if there is ->, it must come before all the dots. So, if one uses map (s->x.y) the above would work, but if map (s->x.y->z) or map (s.a->b->c->d->e) is used, it wouldn't. I'd expect a single while loop that looks through COMPONENT_REFs and MEM_REFs as they appear. Maybe the handle_omp_array_sections_1 MEM_REF case too? Or do you want to have it done incrementally, start with supporting only a single -> first before all the dots and later on add support for the rest? I think the 5.0 and especially 5.1 wording basically says that map clause operand is arbitrary lvalue expression that includes array section support too, so eventually we should just have somewhere in parsing scope a bool whether OpenMP array sections are allowed or not, add OMP_ARRAY_REF or similar tree code for those and after parsing the expression, ensure array sections appear only where they can appear and for a subset of the lvalue expressions where we have decl plus series of -> field or . field or [ index ] or [ array section stuff ] handle those specially. That arbitrary lvalue can certainly be done incrementally. map (foo(123)->a.b[3]->c.d[:7]) and the like. > if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP > && OMP_CLAUSE_MAP_IMPLICIT (c) > && (bitmap_bit_p (&map_head, DECL_UID (t)) > @@ -14802,6 +14814,15 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort) > bias) to zero here, so it is not set erroneously to the pointer > size later on in gimplify.c. */ > OMP_CLAUSE_SIZE (c) = size_zero_node; > + indir_component_ref_p = false; > + if ((ort == C_ORT_ACC || ort == C_ORT_OMP) Same comment about ort tests. > + && TREE_CODE (t) == COMPONENT_REF > + && TREE_CODE (TREE_OPERAND (t, 0)) == MEM_REF) > + { > + t = TREE_OPERAND (TREE_OPERAND (t, 0), 0); > + indir_component_ref_p = true; > + STRIP_NOPS (t); > + } Again, this can handle only a single -> > @@ -42330,16 +42328,10 @@ cp_parser_omp_target (cp_parser *parser, cp_token *pragma_tok, > cclauses[C_OMP_CLAUSE_SPLIT_TARGET] = tc; > } > } > - tree stmt = make_node (OMP_TARGET); > - TREE_TYPE (stmt) = void_type_node; > - OMP_TARGET_CLAUSES (stmt) = cclauses[C_OMP_CLAUSE_SPLIT_TARGET]; > - c_omp_adjust_map_clauses (OMP_TARGET_CLAUSES (stmt), true); > - OMP_TARGET_BODY (stmt) = body; > - OMP_TARGET_COMBINED (stmt) = 1; > - SET_EXPR_LOCATION (stmt, pragma_tok->location); > - add_stmt (stmt); > - pc = &OMP_TARGET_CLAUSES (stmt); > - goto check_clauses; > + c_omp_adjust_map_clauses (cclauses[C_OMP_CLAUSE_SPLIT_TARGET], true); > + finish_omp_target (pragma_tok->location, > + cclauses[C_OMP_CLAUSE_SPLIT_TARGET], body, true); What is the advantage of finish_omp_target. Perhaps the check_clauses label can be renamed and more things common to both paths moved after the label if needed, but as long as it isn't something also called during instantiation, I find it cleaner to do it in cp_parser_omp_target at one place. The reason for e.g. finish_omp_parallel is that it is called from both parsing and instantiation. > --- a/gcc/cp/semantics.c > +++ b/gcc/cp/semantics.c > @@ -4990,6 +4990,9 @@ handle_omp_array_sections_1 (tree c, tree t, vec &types, > { > if (error_operand_p (t)) > return error_mark_node; > + if ((ort == C_ORT_ACC || ort == C_ORT_OMP) See above about ort. Declare simd only allows uniform, linear, aligned, simdlen, inbranch and notinbranch clauses and none of those support array sections. > + && TREE_CODE (t) == FIELD_DECL) > + t = finish_non_static_data_member (t, NULL_TREE, NULL_TREE); handle_omp_array_sections_1 already has recent: if (TREE_CODE (t) == FIELD_DECL && (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_AFFINITY || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND)) ret = finish_non_static_data_member (t, NULL_TREE, NULL_TREE); so shouldn't that be extended to map/to/from clauses too? And I guess we should check reduction/in_reduction/task_reduction clauses too. > @@ -9003,6 +9037,493 @@ finish_omp_construct (enum tree_code code, tree body, tree clauses) > return add_stmt (stmt); > } > > +/* Used to walk OpenMP target directive body. */ > + > +struct omp_target_walk_data > +{ > + tree current_object; > + bool this_expr_accessed; > + > + hash_map ptr_members_accessed; > + hash_set lambda_objects_accessed; > + > + tree current_closure; > + hash_set closure_vars_accessed; > + > + hash_set local_decls; > +}; > + > +static tree > +finish_omp_target_clauses_r (tree *tp, int *walk_subtrees, void *ptr) > +{ > + tree t = *tp; > + struct omp_target_walk_data *data = (struct omp_target_walk_data *) ptr; > + tree current_object = data->current_object; > + tree current_closure = data->current_closure; This is something that we'll eventually need to do e.g. for declare mapper in all the 3 FEs, gather what variables might need to be mapped and for all their types look up the mappers (recursively for nested types and for all types mentioned in those declare mappers etc.) and remember that somehow until gimplification. If it is only preliminary and covers might appear rather than appears, I think it should be fine. What this routine does is ultimate, if you see this somewhere, you say it is accessed, if you see a lambda, again, it has to be accessed etc. I'm afraid that is unsafe though. The IL at this point isn't folded yet, one could have sizeof (this) or other unevaluated context appear there, or something could appear in a private clause on some inner construct that doesn't imply an access on the outer target, etc. So, I think either this function would need to be more careful, especially for nested OpenMP constructs, or can't it be done through langhooks at gimplification time when we should know exactly what appears and what doesn't in the body? > + if (TREE_TYPE(t) && LAMBDA_TYPE_P (TREE_TYPE (t))) Formatting, missing space before (. > + for (hash_set::iterator i = data.closure_vars_accessed.begin (); > + i != data.closure_vars_accessed.end (); ++i) > + { > + tree orig_decl = *i; > + tree closure_expr = DECL_VALUE_EXPR (orig_decl); > + > + if (TREE_CODE (TREE_TYPE (orig_decl)) == POINTER_TYPE) > + { > + /* this-pointer is processed outside this loop. */ > + if (operand_equal_p (closure_expr, omp_target_this_expr)) > + continue; > + > + tree c = build_omp_clause (loc, OMP_CLAUSE_MAP); > + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ALLOC); > + OMP_CLAUSE_DECL (c) > + = build_indirect_ref (loc, closure_expr, RO_UNARY_STAR); > + OMP_CLAUSE_SIZE (c) = size_zero_node; > + OMP_CLAUSE_MAP_MAYBE_ZERO_LENGTH_ARRAY_SECTION (c) = 1; > + new_clauses.safe_push (c); > + > + c = build_omp_clause (loc, OMP_CLAUSE_MAP); > + OMP_CLAUSE_SET_MAP_KIND > + (c, GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION); > + OMP_CLAUSE_DECL (c) = closure_expr; > + OMP_CLAUSE_SIZE (c) = size_zero_node; > + new_clauses.safe_push (c); > + } > + else if (TREE_CODE (TREE_TYPE (orig_decl)) == REFERENCE_TYPE) > + { > + tree c = build_omp_clause (loc, OMP_CLAUSE_MAP); > + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_TO); > + OMP_CLAUSE_DECL (c) > + = build1 (INDIRECT_REF, > + TREE_TYPE (TREE_TYPE (closure_expr)), > + closure_expr); > + OMP_CLAUSE_SIZE (c) > + = TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (closure_expr))); > + new_clauses.safe_push (c); > + > + c = build_omp_clause (loc, OMP_CLAUSE_MAP); > + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ALWAYS_POINTER); > + OMP_CLAUSE_DECL (c) = closure_expr; > + OMP_CLAUSE_SIZE (c) = size_zero_node; > + new_clauses.safe_push (c); Is it guaranteed everything added here can't have an explicit map clause already? Jakub