On Fri, 23 Sep 2022 14:10:51 +0200 Tobias Burnus wrote: > Hi Julian and Jakub, hi all, > > On 23.09.22 09:29, Julian Brown wrote: > > How about this version? (Re-tested.) > > Some more generic (pre)remarks – not affecting the patch code, > but possibly the commit log message: > > > This follows OMP 5.0, 2.19.7.1 "map Clause": > > which is also in "OMP 5.2, 5.8.3 map Clause [152:1-4]". It might > make sense to add this ref in addition (or instead): > > > "If a list item in a map clause is an associated pointer and the > > pointer is not the base pointer of another list item in a map > > clause on the same construct, then it is treated as if its pointer > > target is implicitly mapped in the same clause. For the purposes of > > the map clause, the mapped pointer target is treated as if its base > > pointer is the associated pointer." I've changed the wording in the commit log text... > Thus, the following restriction was proposed for OpenMP 6.0 (TR11): > > "The association status of a list item that is a pointer must not be > undefined unless it is a structure component and it results from a > predefined default mapper." > > which makes my example invalid. (Add some caveat here about TR11 not > yet being released and also TRs being not final named-version > releases.) (But not this bit, for now.) > > and then instead we should follow: > > > > "If the structure sibling list item is a pointer then it is > > treated as if its association status is undefined, unless it > > appears as the base pointer of another list item in a map clause on > > the same construct." > > > This wording disappeared in 5.1 due to some cleanup (cf. Issue 2152, > which has multiple changes; this one is Pull Req. 2379). > > I think the matching current / OpenMP 5.2 wording (5.8.3 map Clause > [152:5-8, 11-13 (,14-16)]) is > > "For map clauses on map-entering constructs, if any list item has a > base pointer for which a corresponding pointer exists in the data > environment upon entry to the region and either a new list item or > the corresponding pointer is created in the device data environment > on entry to the region, then: (Fortran) > 1. The corresponding pointer variable is associated with a pointer > target that has the same rank and bounds as the pointer target of the > original pointer, such that the corresponding list item can be > accessed through the pointer in a target region. ..." > > I think here 'a new list item ... is created ... on entry' applies. > However, this should not affect what you wrote later on. I changed the text here too. > > But, that's not implemented quite right at the moment [...] > > The solution is to detect when we're mapping a smaller part of the > > array (or a subcomponent) on the same directive, and only map the > > descriptor in that case. So we get mappings like this instead: > > > > map(to: tvar%arrptr) --> > > GOMP_MAP_ALLOC tvar%arrptr (the descriptor) > > > > map(tofrom: tvar%arrptr(3:8) --> > > GOMP_MAP_TOFROM tvar%arrptr%data(3) (size 8-3+1, etc.) > > GOMP_MAP_ALWAYS_POINTER tvar%arrptr%data (bias 3, etc.) > > (I concur.) Thank you! > > --- a/gcc/fortran/trans-openmp.cc > > +++ b/gcc/fortran/trans-openmp.cc > > ... > > @@ -2470,22 +2471,18 @@ gfc_trans_omp_array_section (stmtblock_t > > *block, gfc_omp_namelist *n, } > > if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (decl))) > > { > > ... > > + if (ptr_kind != GOMP_MAP_ALWAYS_POINTER) > > { > > ... > > + /* For OpenMP, the descriptor must be mapped with its > > own explicit > > + map clause (e.g. both "map(foo%arr)" and > > "map(foo%arr(:))" must > > + be present in the clause list if "foo%arr" is a > > pointer to an > > + array). So, we don't create a GOMP_MAP_TO_PSET node > > here. */ > > + node2 = build_omp_clause (input_location, > > OMP_CLAUSE_MAP); > > + OMP_CLAUSE_SET_MAP_KIND (node2, GOMP_MAP_TO_PSET); > > I found the last sentence of the comment and the set_map_kind > confusing: The comment says no MAP_TO_PSET and the SET_MAP_KIND use > it. > > I wonder whether that should be something like 'if (openacc)' instead, > which kind of matches the first way gfc_trans_omp_array_section is > called: I agree it was confusing -- I've tweaked the wording of the comment. The condition changes in the "address tokenization" follow-up patch anyway. > inner, element, kind, node, node2, node3, > node4); > However, there is also a second call to it: > > /* An array element or array section which is not > part of a derived type, etc. */ > ... > gomp_map_kind k = GOMP_MAP_POINTER; > ... > gfc_trans_omp_array_section (block, n, decl, > element, k, node, node2, node3, node4); > And without following all 'if' conditions through, I don't see why > that should be handled differently. GOMP_MAP_POINTER is used for non-component accesses -- those get the GOMP_MAP_TO_PSET mapping in gfc_trans_omp_array_section. (Some of these conditions are based on perhaps-not-quite obvious implications, but again, they change in the follow-up patch mentioned above anyway.) > > + /* We're only interested in cases where we have an expression, > > e.g. a > > + component access. */ > > + if (n->expr && n->expr->symtree) > > + use_sym = n->expr->symtree->n.sym; > > The second part looks unsafe in light of 'lvalues'. The next OpenMP > version will likely permit: > "A locator list item is a variable list item, a function reference > with data pointer result, or a reserved locator." > > Thus, permitting 'map( f(cnt=4))' for a function that returns a data > pointer like interface > function f(cnt) result(res) > integer :: cnt > real, pointer :: res(:,:) > end > end interface > > (In Fortran, referencing 'f' counts as variable. However, contrary to > C/C++, 'f()%comp or 'f()(1:4)', i.e. component and array reverences, > are not permitted.) > > Thus, it seems to make more sense to have n->expr->expr_type == > EXPR_VARIABLE as the symtree is also set for EXPR_FUNCTION and > EXPR_COMPCALL. The later is something like dt%f(5) where 'dt' is a > variable of derived type and 'f' is a variable bound to the derived > type. – I think it might also be set for PARAMETER, but I am not sure > until which point. I added n->expr->expr_type == EXPR_VARIABLE to the condition -- I think that should suffice for now? > > + if (!n2->expr || !n2->expr->symtree) > > + continue; > Likewise. > > + /* If the last reference is a pointer to > > a derived > > + type ("foo%dt_ptr"), check if any > > subcomponents > > + of the same derived type member are > > being mapped > > + elsewhere in the clause list > > ("foo%dt_ptr%x", > > + etc.). If we have such subcomponent > > mappings, > > + we only create an ALLOC node for the > > pointer > > + itself, and inhibit mapping the whole > > derived > > + type. */ > > Does the current code handle also the following? > > i = 1; j = 2 > map (foo(i)%dt_ptr(1:3), foo(j)%dt_ptr) Good catch! In that gfc_dep_resolver considers those terms to have a dependency, and that triggers the mapping node transformation. But I don't think OpenMP allows you to write this: IIUC if "foo" is an array, you're not allowed to separately map two parts of the array because of (OpenMP 5.2, "5.8.3 map Clause"): "Two list items of the map clauses on the same construct must not share original storage unless they are the same list item or unless one is the containing structure of the other." and, "If a list item is an array section, it must specify contiguous storage." and maybe also (for different directive arrangements), "If an array appears as a list item in a map clause, multiple parts of the array have corresponding storage in the device data environment prior to a task encountering the construct associated with the map clause, and the corresponding storage for those parts was created by maps from more than one earlier construct, the behavior is unspecified." One thing that *does* work is a similar test to yours but with "i" and "j" pointing to the *same* location. That needs the "address tokenization" patch to be applied too, though. (I added a new test to this version of the patch.) (If multiple variable indices to the same struct array component *were* allowed, that'd cause serious problems for the struct sibling list building code -- it'd no longer be possible to sort members statically. And if different "names" for the same blocks of host memory were allowed, e.g. allowing a pointer and a pointed-to block to be mapped using different variables, that'd imply a requirement to compare each clause against each other clause at runtime -- a quadratic number of tests, probably.) > Note: I have not thought about validity nor checked your code, but it > does not seem to be completely odd code to write. > > A similar mean way to write code would be: > > integer, target :: A(5) > integer, pointer :: p(:), p2(:) > type(t) :: var > > allocate(p2(1:20)) > p => A > var%p2 => p2 > !$omp target map(A(3:4), p2(4:8), p, var%p2) > .... > !$omp end target > > which has a similar issue – it is not clear from the syntax whether > p's or var%p2's pointer target has been mapped or not. Again, I don't think you're allowed to write that: that's "different list items" sharing the same "original storage", IIUC. (It'd be nice to diagnose it at compile time, but that's probably not that easy...) > I don't currently see whether that's handled or not - but I fear it > is not. All this seems to points to first handle all non-pointer > variables, then all with tailing array refs and only then the rest - > and implicit mapping last (followed by use_device_{ptr,addr}). I think I'd need that plan explained a bit more verbosely! But anyway, hopefully it's not necessary. Attached patch re-tested with offloading to NVPTX. OK? Thanks, Julian