Hi Tobias! On 2019-12-18T13:36:29+0100, Tobias Burnus <tob...@codesourcery.com> wrote: > libgomp/target.c's gomp_map_vars_internal: it now uses the normal code > path in the upper loop, except that one directly bails out when the > 'key' has not been found (skipping the adjacent MAP_POINTER as well). > The 'case' in the second loop is only reached, if tgt[i]->key == NULL > (i.e. if not present) and one can unconditionally skip here. — This > seems to be cleaner and should avoid some confusions :-)
Oh, great! It seems that you managed to de-cypher what my brain (or was it my gut feeling?) told me to write down in these TODO comments that I had added. ;-) I have not now reviewed the details, but from the structure, your changes looks good, and if it work, all the better. I note you're building up a "dangerous" ;-) level of understanding of OMP internals! :-) > GOMP_MAP_POINTER, following MAP_IF_PRESENT: I am not sure about this. So, what does a 'GOMP_MAP_POINTER' following a non-present 'GOMP_MAP_IF_PRESENT' mean -- is this 'GOMP_MAP_POINTER' operation actually a no-op then, given that in the non-present case we'll just use the host pointer? But if it is a no-op, should we then just let the mapping code execute these 'GOMP_MAP_POINTER' operation, instead of adding special-case code to skip them? Are there any interactions with the OpenACC 2.6 manual deep copy implementation maybe? > The testsuite digests both mapping and skipping the map pointer. It > looks a tad cleaner to avoid mapping the pointer (if the var is not > present) – saving also few bytes and cpu cycles. On the down side, it > adds an order dependence assumption, namely assuming that the > MAP_POINTER after 'no_create'/MAP_IF_PRESENT always belongs to > no_create. – [This patch follows the original patch and skips the > map_pointer.] Per his OpenACC 2.6 manual deep copy work, Julian has indeed established that a 'GOMP_MAP_POINTER' is "only expected after some other mapping"; see "case GOMP_MAP_POINTER" in <65540b92dff74db1f15af930f87f7096d03e7efe.1576648001.git.julian@codesourcery.com">http://mid.mail-archive.com/65540b92dff74db1f15af930f87f7096d03e7efe.1576648001.git.julian@codesourcery.com>, for example. See also <https://gcc.gnu.org/wiki/LibgompPointerMappingKinds> "unfinished notes on pointer mapping kinds" that Julian created. The question then is, is it (a) correct (also per the OpenACC 2.6 manual deep copy requirements) to skip these 'GOMP_MAP_POINTER' after 'GOMP_MAP_IF_PRESENT', and (b) only 'GOMP_MAP_POINTER' or also other "variants", and/or (c) not do that skipping? (For avoidance of doubt: this is fine to resolve later, given that it may depend on the pending OpenACC 2.6 manual deep copy, and doesn't seem to cause any issues at present.) > Otherwise, except for added acc_is_present calls to no_create-3.c to > check that no_create does not cause mapping and applying your/Thomas's > patches, it matches my previous version, which was OK'ed. — Hence, I > intent to commit it tomorrow, unless there are further comments. ACK. > On 12/17/19 8:11 PM, Tobias Burnus wrote: >> On 12/3/19 4:16 PM, Thomas Schwinge wrote: >>> Another thing: I've added just another little bit of testsuite >>> coverage, and another thing broke. See "TODO" in attached incremental >>> patch. […] >> Files included, the other issue was XFAILed by you (and hence passed). >> A fix for that issue is: >> https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01135.html — and a >> completely separate issue. (That patch is small, very localized and >> orthogonal to this patch.) ACK, that's for later. >>> The incremental Fortran test case changes have bene done in a rush; not >>> sure if they make much sense, or should see some further work applied to >>> them. >> >> I think one can do more, but they are fine. I am not 100% sure how to >> read the following: >> >> ! The no_create clause is meant for partially shared-memory >> machines. This >> ! test is written to work on non-shared-memory machines, though this >> is not >> ! necessarily a useful way to use the no_create clause in practice. (We inherited that from somebody else. I too didn't quickly understand that.) >> !$acc parallel !no_create (var) >> >> First, why is 'no_create(var)' now commented? – For this code, it >> should really work both ways and independent whether commented boils >> down to 'copy' (currently) or 'present' (with my other patch, linked >> above). If I remember correctly (remember: "done in a rush"), I think that was my rationale: we should get kind-of an implicit 'no_create' here. ..., and then, learned something new this evening: > .../testsuite/libgomp.oacc-fortran/no_create-1.f90 | 39 ++++++++++ > .../testsuite/libgomp.oacc-fortran/no_create-2.f90 | 90 > ++++++++++++++++++++++ > .../testsuite/libgomp.oacc-fortran/no_create-3.F90 | 39 ++++++++++ > --- /dev/null > +++ b/libgomp/testsuite/libgomp.oacc-fortran/no_create-3.F90 Why is this upper-case '.F90' when others are lower-case '.f90'? > @@ -0,0 +1,39 @@ > +! { dg-do run } > + > +program main > + use iso_c_binding, only: c_sizeof > + use openacc, only: acc_is_present > + implicit none > + integer i > + integer, parameter :: n = 100 > + real*4 b(n), c(n) > + real :: d(n), e(n) > + common /BLOCK/ d, e > + > + !$acc enter data create(b) create(d) > + > + if (.not. acc_is_present(b, c_sizeof(b))) stop 1 > + if (.not. acc_is_present(d, c_sizeof(d))) stop 2 > +#if !ACC_MEM_SHARED > +[...] Aha! Grüße Thomas
signature.asc
Description: PGP signature
