Re: [PATCH 2/2] OpenMP/Fortran: Enforce component order when mapping allocatable DT [PR120505]

Paul-Antoine Arras wrote:
> This is a follow-up to r16-5789-g05c2ad4a2e7104.
>
> Consider the following code, assuming tiles is allocatable:
>
> type t
>   integer, allocatable :: den1(:,:), den2(:,:)
> end type t
> [...]
> !$omp target enter data map(var%tiles(1)%den2, var%tiles(1)%den1)
>
> r16-5789-g05c2ad4a2e7104 allowed mapping several components from the same
> allocatable derived type, provided they are in the right order in user code.
> This patch relaxes this constraint by computing offsets and sorting to-be-mapped
> components at gimplification time.
> 	PR fortran/120505
>
> gcc/ChangeLog:
>
> 	* gimplify.cc (omp_accumulate_sibling_list): When the containing struct
> 	is a Fortran array descriptor, sort mapped components by offset.
>
> libgomp/ChangeLog:
>
> 	* testsuite/libgomp.fortran/map-subarray-12.f90: New test.
>
> gcc/testsuite/ChangeLog:
>
> 	* gfortran.dg/gomp/map-subarray-4.f90: New test.

LGTM – however, I think it would be good if at least one of the 
testcases could be made a bit more through/robust:

Currently, only two elements are checked for being in order – I think it 
would be better to check for a handful, added to the map clauses in 
random order (and possibly skipping one or two).

Additionally, it might help to add a comment like 'sort mapped 
components by offset' in the 'if (has_descriptor)' block to make it 
easier to follow that the code does.

Otherwise, the patch the patch seems to be fine to me.

Sorry for the slow review!

Tobias