On 2020/10/29 7:49 PM, Jakub Jelinek wrote:
> On Wed, Oct 28, 2020 at 06:32:21PM +0800, Chung-Lin Tang wrote:
>>>> @@ -8958,25 +9083,20 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>>>>    	      /* An "attach/detach" operation on an update directive should
>>>>    		 behave as a GOMP_MAP_ALWAYS_POINTER.  Beware that
>>>>    		 unlike attach or detach map kinds, GOMP_MAP_ALWAYS_POINTER
>>>>    		 depends on the previous mapping.  */
>>>>    	      if (code == OACC_UPDATE
>>>>    		  && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH_DETACH)
>>>>    		OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ALWAYS_POINTER);
>>>> -	      if (gimplify_expr (pd, pre_p, NULL, is_gimple_lvalue, fb_lvalue)
>>>> -		  == GS_ERROR)
>>>> -		{
>>>> -		  remove = true;
>>>> -		  break;
>>>> -		}
>>> So what gimplifies those now?
>>
>> They're gimplified somewhere during omp-low now.
>> (some gimplify scan testcases were adjusted to accommodate this change)
>>
>> I don't remember the exact case I encountered, but there were some issues with gimplified
>> expressions inside the map clauses making some later checking more difficult. I haven't seen
>> any negative effect of this modification so far.
> 
> I don't like that, it goes against many principles, gimplification really
> shouldn't leave around non-GIMPLE IL.
> If you need to compare same expression or same expression bases later,
> perhaps detect the equalities during gimplification before actually gimplifying the
> clauses and ensure they are gimplified to the same expression or are using
> same base (e.g. by adding SAVE_EXPRs or TARGET_EXPRs before the
> gimplification).

I have moved that same gimplify_expr call down to below the processing block,
and things still work as expected. My aforementioned gimple-scan-test modifications
have all been reverted, and all original tests still pass correctly.

Thanks,
Chung-Lin

	gcc/
	* gimplify.c (is_or_contains_p): New static helper function.
	(omp_target_reorder_clauses): New function.
	(gimplify_scan_omp_clauses): Add use of omp_target_reorder_clauses to
	reorder clause list according to OpenMP 5.0 rules. Add handling of
	GOMP_MAP_ATTACH_DETACH for OpenMP cases.
	* omp-low.c (is_omp_target): New static helper function.
	(scan_sharing_clauses): Add scan phase handling of GOMP_MAP_ATTACH/DETACH
	for OpenMP cases.
	(lower_omp_target): Add lowering handling of GOMP_MAP_ATTACH/DETACH for
	OpenMP cases.

	gcc/testsuite/
	* c-c++-common/gomp/clauses-2.c: Remove dg-error cases now valid.
	* gfortran.dg/gomp/map-2.f90: Likewise.
	* c-c++-common/gomp/map-5.c: New testcase.