On Wed, 7 Dec 2022 17:31:20 +0100
Tobias Burnus <tobias@codesourcery.com> wrote:

> Hi Julian,
> 
> I think this patch is OK; however, at least for gimplify.cc Jakub
> needs to have a second look.

Thanks for the review!  Here's a new version that hopefully addresses
your comments.  (The gimplify bits change a bit more in this version!)

> As remarked for the 2/4 patch, I believe mapping 'map(tofrom:
> var%f(2:3))' should work without explicitly mapping 'map(tofrom:
> var%f)' (→ [TR11 157:21-26] (approx. [5.2 154:22-27], [5.1
> 352:17-22], [5.0 320:22-27]). →
> https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608100.html
> (+ previously in the thread).
> 
> Testing the patch, that seems to work fine (i.e. contrary to C/C++,
> cf. 2/4), which matches the dump and, if I understood correctly, also
> your (Julian's) expectation. Thus, no need to modify the code part.

That change is undone.

> Regarding the testcases:
> * I would prefer if you don't modify the existing
> libgomp.fortran/struct-elem-map-1.f90 testcase; However, you could
> add your version as another variant ('subroutine nine()',
> 'four_var()' or what's the next free name, possibly with a comment
> telling that it is 'four()' but with an added explicit basepointer
> mapping.).

I've added new functions "nine" through "twelve" with the added base
pointer.

> * As the new version should map *less*, I wonder whether some
> -fdump-tree-{original,gimple,omplower} scan-dump-tree checks would be
> useful besides testing whether it works at run time. (Your decision
> regarding which tree, which testcases and whether at all.)

A couple added...

> * Likewise, maybe a 'target enter/exit data' check? However, you
> might very well run into my 'omp target data exit' issue, cf.
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/604887.html
> (needs to be revised based on Jakub's comments; I think those were on
> IRC only – the problem is that not only 'alloc' is affected but also
> 'from' etc.)

Hmm -- this wasn't quite as straightforward as it probably should have
been!  This version of the patch fixes a couple of bugs with "enter
data" and "exit data" directives I found when adding a new test
(map-subarray-8.f90).

(I'm not sure if this patch fixes all the problems you saw previously
with "alloc", etc. for enter/exit data -- there might still be work to
do there.)

Re-tested with offloading to NVPTX.  Does this look OK now?

Thanks,

Julian