Hi Julian! On 2019-11-09T01:04:21+0000, Julian Brown <jul...@codesourcery.com> wrote: > This patch fixes an issue I noticed when investigating an answer > for Thomas's question about device pointer return values in: > > https://gcc.gnu.org/ml/gcc-patches/2019-10/msg02260.html > > It looks to me like the return value for the present case is wrong in > the existing code: in case of a acc_pcopyin or similar call that refers > to a subarray of a larger block already mapped on the target, the > device pointer return value will be the start of the larger block, not > of the subarray being copied.
Note that I've filed <https://gcc.gnu.org/PR92511> "[OpenACC] Support subset subarray mappings", so please reference that one in the ChangeLog/commit log. Principal ACK for that problem, and it's solution ('libgomp/oacc-mem.c:present_create_copy' 'if (n)' change). Then, I was confused, because I couldn't really find wording in the OpenACC specification that explicitly permits such things. But given that, for example, in OpenACC 2.7, 3.2.20. "acc_copyin", 'acc_copyin' is described to be "equivalent to the 'enter data' directive with a 'copyin' clause", and the latter supposedly (?) does allow such "subset subarray mappings", and in 2.7.6. "copyin clause" it is said that "An 'enter data' directive with a 'copyin' clause is functionally equivalent to a call to the 'acc_copyin' API routine", that's probably motivation enough to fix the latter to conform what the former supposedly already is allowing (though not implementing by means of 'enter data copyin' just calling 'acc_copyin' etc. I see that 2.7.6. "copyin clause" also states that "The restrictions regarding subarrays in the present clause apply to this clause", which per 2.7.4. "present clause" is that "If only a subarray of an array is present in the current device memory, the 'present' clause must specify the same subarray, or a subarray that is a proper subset of the subarray in the data lifetime". From that we probably are to deduce that it's fine the other way round (as you've argued): if a subarray of an array (or, the whole array) is present in the current device memory, the 'present' clause may specify the same subarray, or a subarray that is a proper subset of the subarray in the data lifetime (my words). Unless you object to that, we shall (later) try to get the clarified/amended in the OpenACC specification. Indeed I am confirming that such subset subarray mappings do work fine with PGI 19.4 and 19.10 -- but only when using OpenACC directives, not necessarily when using OpenACC runtime library calls, huh. (That's not our problem to solve, of course, and under the assumption that my test case has actually been valid.) Later (not now), we should then also add corresponding testing for actual 'data' etc. constructs being nested in that way. > The attached patch corrects this issue, and also relaxes a restriction > on acc_delete, acc_copyout (etc.) to allow them to unmap/copyout > subarrays of a larger block already present on the target. There's no > particular reason to disallow that, as far as I can tell. (That's where PGI fails at runtime, but I have not analyzed how exactly this fails -- let's first clarify that with OpenACC Technical Committee, later on.) > This is > necessary to allow the new tests included with this patch to pass, and > a couple of existing "shouldfail" tests no longer fail, and have been > adjusted accordingly. These should then actually be removed, or re-written, because in their current form they no longer make much sense, as far as I can tell: For example, 'libgomp.oacc-c-c++-common/lib-22.c': acc_copyin (h, N); ... followed by: acc_copyout (h + 1, N - 1); ... is now meant to no longer abort with a "surrounds2" message, but instead we now expect success, and '!acc_is_present'. I'll take care of that later on -- I have some more tests to add anyway. > It's still an error to try to copy data beyond > the bounds of a mapped block, and other existing tests cover those > cases. ACK. > The calculation for the return value for the non-present case of > present_create_copy has also been adjusted in anticipation of a new > version of the above-linked patch. But please back out this one, for it's not related to this bug fix, and we shall take care of that in a later patch. (No need for you to re-post that one just for this.) > Tested with offloading to nvptx. OK for trunk? I'm see C++ compilation failures the new libgomp test cases; OK with these resolved. To record the review effort, please include "Reviewed-by: Thomas Schwinge <tho...@codesourcery.com>" in the commit log, see <https://gcc.gnu.org/wiki/Reviewed-by>. Grüße Thomas
signature.asc
Description: PGP signature
