Hi Julian! On 2019-10-03T09:35:04-0700, Julian Brown <jul...@codesourcery.com> wrote: > This patch has been broken out of the patch supporting OpenACC 2.6 manual > deep copy last posted here: > > https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01084.html
Thanks.
> a couple of
> tests need fixing also
Let's look at these first, and independently.
The overall goal not being to bend test cases until they (again) work,
but rather to verify what they're testing, so that they're valid OpenACC
code, or if not that, then they're testing specifics of the GCC
implementation (for example, the 'dg-shouldfail' test cases).
> * testsuite/libgomp.oacc-c-c++-common/context-2.c: Use correct API to
> deallocate acc_copyin'd data.
> * testsuite/libgomp.oacc-c-c++-common/context-4.c: Likewise.
> --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/context-2.c
> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/context-2.c
> + acc_delete (&h_X[0], N * sizeof (float));
> + acc_delete (&h_Y1[0], N * sizeof (float));
> +
> free (h_X);
> free (h_Y1);
> free (h_Y2);
>
> - acc_free (d_X);
> - acc_free (d_Y);
> --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/context-4.c
> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/context-4.c
> + acc_delete (&h_X[0], N * sizeof (float));
> + acc_delete (&h_Y1[0], N * sizeof (float));
> +
> free (h_X);
> free (h_Y1);
> free (h_Y2);
>
> - acc_free (d_X);
> - acc_free (d_Y);
ACK -- but do we understand why the same shouldn't be applied to the very
similar 'libgomp.oacc-c-c++-common/context-1.c' and
'libgomp.oacc-c-c++-common/context-3.c', too?
I suppose your testing of the "OpenACC reference count overhaul" tripped
over these constructs? (Why just some, then?)
The same pattern ('acc_copyin', 'acc_free') also appears in
'libgomp.oacc-c-c++-common/clauses-1.c', does that also need to be
corrected? Same in 'libgomp.oacc-c-c++-common/lib-13.c' (... where that
test case actually is titled "Check acc_is_present and acc_delete"
instead of "[...] acc_free", huh), 'libgomp.oacc-c-c++-common/lib-14.c',
'libgomp.oacc-c-c++-common/lib-18.c'.
Then, the 'acc_deviceptr', 'acc_unmap_data', 'acc_free' usage in
'libgomp.oacc-c-c++-common/clauses-1.c' also seems strange, as the
respective 'acc_free' argument certainly is not (at least not directly) a
"pointer value that was returned by a call to 'acc_malloc'". Does it
make sense to (continue to) support that, assuming that's how it's
implemented internally, or should these be corrected to valid OpenACC,
too? Same in 'libgomp.oacc-c-c++-common/present-1.c'.
Same in 'libgomp.oacc-c-c++-common/clauses-2.c' (we 'dg-shouldfail'
earlier, but the later code should otherwise be made correct anyway).
Several of these things again in 'libgomp.oacc-c-c++-common/nested-1.c'.
(The other 'libgomp.oacc-c-c++-common/lib-*.c' ones are correctly pairing
'acc_malloc', 'acc_free', as far as I can tell.)
> --- a/libgomp/testsuite/libgomp.oacc-fortran/data-2.f90
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/data-2.f90
> @@ -70,10 +71,14 @@ program test
> end do
> !$acc end parallel
>
> - !$acc exit data copyout (d(1:N)) async
> + !$acc exit data delete (c(1:N)) copyout (d(1:N)) async
> !$acc exit data async
> !$acc wait
ACK, but also it seems to me as if the '!$acc exit data async' (currently
"clause-less") was meant to carry the 'delete (c(1:N))' clause?
> @@ -1,4 +1,5 @@
> ! { dg-do run }
> +! { dg-additional-options "-cpp" }
> [...]
> +#if !ACC_MEM_SHARED
> + if (acc_is_present (c) .eqv. .TRUE.) call abort
> +#endif
;-) Should be able to simplify that one to 'if (acc_is_present (c))', no?
But is that a really useful test here: don't we elsewhere have enough of
such 'acc_is_present' testing? (That is, OK to keep that, but likewise
OK to drop that.)
And, just for background information: per PR84381, it has been suggested
to use the Fortran standard 'stop' (or was it 'error stop'?) instead of
'call abort'. But no need to change that here individually; the libgomp
testsuite still (or, again?) contains a lot of 'call abort'.
> +
> do i = 1, N
> if (d(i) .ne. 4.0) call abort
> end do
..., for example, here. ;-) (For avoidance of doubt, I'm not asking you
to change these now.)
So, please address these items first, as separate "Fix OpenACC test cases
regarding 'acc_malloc', 'acc_free' pairing", and "Fix OpenACC test case
for unstructured data regions" (or similar) commits. If you're confident
you're doing "the obvious", feel free to commit without further review.
Grüße
Thomas
signature.asc
Description: PGP signature
