Hi Jakub,
Thank you for the review.

On 2020/10/13 9:01 PM, Jakub Jelinek wrote:
>>          gcc/c-family/
>>          * c-common.h (c_omp_adjust_clauses): New declaration.
>>          * c-omp.c (c_omp_adjust_clauses): New function.
> Besides the naming, I wonder why is it done in a separate function and so
> early, can't what the function does be done either in
> {,c_}finish_omp_clauses (provided we'd pass separate ORT_OMP vs.
> ORT_OMP_TARGET to it to determine if it is target region vs. anything else),
> or perhaps even better during gimplification (gimplify_scan_omp_clauses)?

I figured that differentiating with something like "C_ORT_OMP_TARGET" could be
more error prone to adjust changes related to C_ORT_OMP across the code, plus
this has the added benefit of sharing a single place of handling logic across C/C++.

You're right about the need for early addressable-marking. Learned that the hard
way, one of my prior attempts tried to place this code somewhere in gimplify,
didn't work.

>>          gcc/cp/
>>          * parser.c (cp_parser_omp_target_data): Add use of
>>          new c_omp_adjust_clauses function. Add GOMP_MAP_ATTACH_DETACH as
>>          handled map clause kind.
>>          (cp_parser_omp_target_enter_data): Likewise.
>> 	(cp_parser_omp_target_exit_data): Likewise.
>> 	(cp_parser_omp_target): Likewise.
>> 	* semantics.c (handle_omp_array_sections): Adjust COMPONENT_REF case to
>> 	use GOMP_MAP_ATTACH_DETACH map kind for C_ORT_OMP region type. Fix
>> 	interaction between reference case and attach/detach.
>> 	(finish_omp_clauses): Adjust bitmap checks to allow struct decl and
>> 	same struct field access to co-exist on OpenMP construct.
> The changelog has some 8 space indented lines.

I'll take care of that in the final git push.

>> +  for (tree c = clauses; c; c = OMP_CLAUSE_CHAIN (c))
>> +    if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
>> +	&& OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER
>> +	&& TREE_CODE (TREE_TYPE (OMP_CLAUSE_DECL (c))) != ARRAY_TYPE)
>> +      {
>> +	tree ptr = OMP_CLAUSE_DECL (c);
>> +	bool ptr_mapped = false;
>> +	if (is_target)
>> +	  {
>> +	    for (tree m = clauses; m; m = OMP_CLAUSE_CHAIN (m))
> Isn't this O(n^2) in number of clauses?  I mean, e.g. for the equality
> comparisons (but see below) it could be dealt with e.g. using some bitmap
> with DECL_UIDs.

At this stage, we really don't assume any ordering of the clauses, nor try to
modify its ordering yet, so the base-pointer map (if it exists) could be any
where in the list (building some "visited set" isn't really suitable here).
I don't think this is really that much an issue of concern though.

>> +	      if (OMP_CLAUSE_CODE (m) == OMP_CLAUSE_MAP
>> +		  && OMP_CLAUSE_DECL (m) == ptr
> Does it really need to be equality?  I mean it will be for
> map(tofrom:ptr) map(tofrom:ptr[:32])
> but what about e.g.
> map(tofrom:structx) map(tofrom:structx.ptr[:32])
> ?  It is true that likely we don't parse this yet though.

The code for COMPONENT_REF based expressions are actually handled quite differently
in gimplify_scan_omp_clauses. Not completely sure there's nothing to handle for the
code in this patch set, but will have to discover such testcases later.

>> +		  && (OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_ALLOC
>> +		      || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_TO
>> +		      || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_FROM
>> +		      || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_TOFROM))
> What about the always modified mapping kinds?

Took care of that.

>> +		{
>> +		  ptr_mapped = true;
>> +		  break;
>> +		}
>> +
>> +	    if (!ptr_mapped
>> +		&& DECL_P (ptr)
>> +		&& is_global_var (ptr)
>> +		&& lookup_attribute ("omp declare target",
>> +				     DECL_ATTRIBUTES (ptr)))
>> +	      ptr_mapped = true;
>> +	  }
>> +
>> +	/* If the pointer variable was mapped, or if this is not an offloaded
>> +	   target region, adjust the map kind to attach/detach.  */
>> +	if (ptr_mapped || !is_target)
>> +	  {
>> +	    OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ATTACH_DETACH);
>> +	    c_common_mark_addressable_vec (ptr);
> Though perhaps this is argument why it needs to be done in the FEs and not
> during gimplification, because it is hard to mark something addressable at
> that point.

Discussed above.

>> --- a/gcc/c/c-typeck.c
>> +++ b/gcc/c/c-typeck.c
>> @@ -13580,16 +13580,17 @@ handle_omp_array_sections (tree c, enum c_omp_region_type ort)
>>   	    break;
>>   	  }
>>         tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c), OMP_CLAUSE_MAP);
>>         if (ort != C_ORT_OMP && ort != C_ORT_ACC)
>>   	OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_POINTER);
>>         else if (TREE_CODE (t) == COMPONENT_REF)
>>   	{
>> -	  gomp_map_kind k = (ort == C_ORT_ACC) ? GOMP_MAP_ATTACH_DETACH
>> -					       : GOMP_MAP_ALWAYS_POINTER;
>> +	  gomp_map_kind k
>> +	    = ((ort == C_ORT_ACC || ort == C_ORT_OMP)
>> +	       ? GOMP_MAP_ATTACH_DETACH : GOMP_MAP_ALWAYS_POINTER);
> So what kind of C_ORT_* would be left after this change?
> C_ORT_*DECLARE_SIMD shouldn't have any kind of array sections in it.
> So maybe just
> 
>>   	  OMP_CLAUSE_SET_MAP_KIND (c2, k);
> OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_ALWAYS_POINTER);
> ?

I have changed this code to just "OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_ATTACH_DETACH);"

>>   	      if (VAR_P (t) || TREE_CODE (t) == PARM_DECL)
>>   		{
>> -		  if (bitmap_bit_p (&map_field_head, DECL_UID (t)))
>> +		  if (bitmap_bit_p (&map_field_head, DECL_UID (t))
>> +		      || bitmap_bit_p (&map_head, DECL_UID (t)))
>>   		    break;
> Shall this change apply to OpenACC too?
> 
>>   		}
>>   	    }
>>   	  if (!VAR_P (t) && TREE_CODE (t) != PARM_DECL)
>>   	    {
>>   	      error_at (OMP_CLAUSE_LOCATION (c),
>>   			"%qE is not a variable in %qs clause", t,
>> @@ -14751,29 +14753,36 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>>   		    error_at (OMP_CLAUSE_LOCATION (c),
>>   			      "%qD appears both in data and map clauses", t);
>>   		  remove = true;
>>   		}
>>   	      else
>>   		bitmap_set_bit (&generic_head, DECL_UID (t));
>>   	    }
>> -	  else if (bitmap_bit_p (&map_head, DECL_UID (t)))
>> +	  else if (bitmap_bit_p (&map_head, DECL_UID (t))
>> +		   && !bitmap_bit_p (&map_field_head, DECL_UID (t)))
> Ditto.  Otherwise, what shall this diagnose now that the restriction that
> the same list item may not appear in multiple clauses is gone.

Thanks for catching this, I've added "C_ORT_OMP" tests to these parts.

Attached is the revised patch, dubbed "v2". Entire patch set re-tested with no regressions
for gcc, g++, gfortran, and libgomp on x86_64-linux with nvptx offloading.

Thanks,
Chung-Lin