Re: [PATCH, v5, OpenMP 5.0] Improve OpenMP target support for C++ [PR92120 v5]

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Nov 16, 2021 at 08:43:27PM +0800, Chung-Lin Tang wrote:
> 2021-11-16  Chung-Lin Tang  <cltang@codesourcery.com>
> 
> 	PR middle-end/92120
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (finish_omp_target): New declaration.
> 	(finish_omp_target_clauses): Likewise.
> 	* parser.c (cp_parser_omp_clause_map): Adjust call to
> 	cp_parser_omp_var_list_no_open to set 'allow_deref' argument to true.
> 	(cp_parser_omp_target): Factor out code, adjust into calls to new
> 	function finish_omp_target.
> 	* pt.c (tsubst_expr): Add call to finish_omp_target_clauses for
> 	OMP_TARGET case.
> 	* semantics.c (handle_omp_array_sections_1): Add handling to create
> 	'this->member' from 'member' FIELD_DECL. Remove case of rejecting
> 	'this' when not in declare simd.
> 	(handle_omp_array_sections): Likewise.
> 	(finish_omp_clauses): Likewise. Adjust to allow 'this[]' in OpenMP
> 	map clauses. Handle 'A->member' case in map clauses. Remove case of
> 	rejecting 'this' when not in declare simd.
> 	(struct omp_target_walk_data): New struct for walking over
> 	target-directive tree body.
> 	(finish_omp_target_clauses_r): New function for tree walk.
> 	(finish_omp_target_clauses): New function.
> 	(finish_omp_target): New function.
> 
> gcc/c/ChangeLog:
> 
> 	* c-parser.c (c_parser_omp_clause_map): Set 'allow_deref' argument in
> 	call to c_parser_omp_variable_list to 'true'.
> 	* c-typeck.c (handle_omp_array_sections_1): Add strip of MEM_REF in
> 	array base handling.
> 	(c_finish_omp_clauses): Handle 'A->member' case in map clauses.
> 
> gcc/ChangeLog:
> 
> 	* gimplify.c ("tree-hash-traits.h"): Add include.
> 	(gimplify_scan_omp_clauses): Change struct_map_to_clause to type
> 	hash_map<tree_operand, tree> *. Adjust struct map handling to handle
> 	cases of *A and A->B expressions. Under !DECL_P case of
> 	GOMP_CLAUSE_MAP handling, add STRIP_NOPS for indir_p case, add to
> 	struct_deref_set for map(*ptr_to_struct) cases. Add MEM_REF case when
> 	handling component_ref_p case. Add unshare_expr and gimplification
> 	when created GOMP_MAP_STRUCT is not a DECL. Add code to add
> 	firstprivate pointer for *pointer-to-struct case.
> 	(gimplify_adjust_omp_clauses): Move GOMP_MAP_STRUCT removal code for
> 	exit data directives code to earlier position.
> 	* omp-low.c (lower_omp_target):
> 	Handle GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION, and
> 	GOMP_MAP_POINTER_TO_ZERO_LENGTH_ARRAY_SECTION map kinds.
> 	* tree-pretty-print.c (dump_omp_clause): Likewise.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.dg/gomp/target-3.c: New testcase.
> 	* g++.dg/gomp/target-3.C: New testcase.
> 	* g++.dg/gomp/target-lambda-1.C: New testcase.
> 	* g++.dg/gomp/target-lambda-2.C: New testcase.
> 	* g++.dg/gomp/target-this-1.C: New testcase.
> 	* g++.dg/gomp/target-this-2.C: New testcase.
> 	* g++.dg/gomp/target-this-3.C: New testcase.
> 	* g++.dg/gomp/target-this-4.C: New testcase.
> 	* g++.dg/gomp/target-this-5.C: New testcase.
> 	* g++.dg/gomp/this-2.C: Adjust testcase.
> 
> include/ChangeLog:
> 
> 	* gomp-constants.h (enum gomp_map_kind):
> 	Add GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION, and
> 	GOMP_MAP_POINTER_TO_ZERO_LENGTH_ARRAY_SECTION map kinds.
> 	(GOMP_MAP_POINTER_P):
> 	Include GOMP_MAP_POINTER_TO_ZERO_LENGTH_ARRAY_SECTION.
> 
> libgomp/ChangeLog:
> 
> 	* libgomp.h (gomp_attach_pointer): Add bool parameter.
> 	* oacc-mem.c (acc_attach_async): Update call to gomp_attach_pointer.
> 	(goacc_enter_data_internal): Likewise.
> 	* target.c (gomp_map_vars_existing): Update assert condition to
> 	include GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION.
> 	(gomp_map_pointer): Add 'bool allow_zero_length_array_sections'
> 	parameter, add support for mapping a pointer with NULL target.
> 	(gomp_attach_pointer): Add 'bool allow_zero_length_array_sections'
> 	parameter, add support for attaching a pointer with NULL target.
> 	(gomp_map_vars_internal): Update calls to gomp_map_pointer and
> 	gomp_attach_pointer, add handling for
> 	GOMP_MAP_ATTACH_ZERO_LENGTH_ARRAY_SECTION, and
> 	GOMP_MAP_POINTER_TO_ZERO_LENGTH_ARRAY_SECTION cases.
> 	* testsuite/libgomp.c++/target-23.C: New testcase.
> 	* testsuite/libgomp.c++/target-lambda-1.C: New testcase.
> 	* testsuite/libgomp.c++/target-lambda-2.C: New testcase.
> 	* testsuite/libgomp.c++/target-this-1.C: New testcase.
> 	* testsuite/libgomp.c++/target-this-2.C: New testcase.
> 	* testsuite/libgomp.c++/target-this-3.C: New testcase.
> 	* testsuite/libgomp.c++/target-this-4.C: New testcase.
> 	* testsuite/libgomp.c++/target-this-5.C: New testcase.

> +/* Used to walk OpenMP target directive body.  */
> +
> +struct omp_target_walk_data
> +{
> +  tree current_object;
> +  bool this_expr_accessed;
> +
> +  hash_map<tree, tree> ptr_members_accessed;
> +  hash_set<tree> lambda_objects_accessed;
> +
> +  tree current_closure;
> +  hash_set<tree> closure_vars_accessed;
> +
> +  hash_set<tree> local_decls;

Can you please add short comments above the members describing
what they are for?
> +};
> +
> +static tree
> +finish_omp_target_clauses_r (tree *tp, int *walk_subtrees, void *ptr)

And add function comments above here.
> +
> +void
> +finish_omp_target_clauses (location_t loc, tree body, tree *clauses_ptr)

And here.

> +
> +tree
> +finish_omp_target (location_t loc, tree clauses, tree body, bool combined_p)

And here?

> +	  if (allow_zero_length_array_sections)
> +	    {
> +	      /* When allowing attachment to zero-length array sections, we
> +		 allow attaching to NULL pointers when the target region is not
> +		 mapped.  */
> +	      data = 0;
> +	    }

No {}s around single statement if body.

Otherwise LGTM.

	Jakub