Hi Julian!
On 2020-06-16T15:39:42-0700, Julian Brown <jul...@codesourcery.com> wrote:
> This is a fix for the pointer (or array) size inadvertently being used
> for the bias of attach and detach clauses (PR95270)
Thanks for looking into that one, which had caused my some gray hair.
> for C and C++.
That means, there is no such problem for Fortran? (I haven't run into
one, just curious.)
> OK?
In principle, yes, for master and releases/gcc-10 branches, but please
incorporate the following items:
> PR middle-end/95270
>
> gcc/c/
> * c-typeck.c (c_finish_omp_clauses): Set OMP_CLAUSE_SIZE (bias) to zero
> for standalone attach/detach clauses.
>
> gcc/cp/
> * semantics.c (finish_omp_clauses): Likewise.
>
> gcc/testsuite/
> * c-c++-common/goacc/mdc-1.c: Update expected dump output for zero
> bias.
> ---
> gcc/c/c-typeck.c | 8 ++++++++
> gcc/cp/semantics.c | 8 ++++++++
> gcc/testsuite/c-c++-common/goacc/mdc-1.c | 14 +++++++-------
> 3 files changed, 23 insertions(+), 7 deletions(-)
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -14533,6 +14533,10 @@ c_finish_omp_clauses (tree clauses, enum
> c_omp_region_type ort)
> }
> if (c_oacc_check_attachments (c))
> remove = true;
> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH
> + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH))
> + OMP_CLAUSE_SIZE (c) = size_zero_node;
> break;
> }
> if (t == error_mark_node)
> @@ -14546,6 +14550,10 @@ c_finish_omp_clauses (tree clauses, enum
> c_omp_region_type ort)
> remove = true;
> break;
> }
> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH
> + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH))
> + OMP_CLAUSE_SIZE (c) = size_zero_node;
> if (TREE_CODE (t) == COMPONENT_REF
> && OMP_CLAUSE_CODE (c) != OMP_CLAUSE__CACHE_)
> {
I cannot comment if these two code paths are good places (and the only
ones) that need to set 'OMP_CLAUSE_SIZE', so I'll trust you've found the
best/all places.
Does that override an 'OMP_CLAUSE_SIZE' that was set earlier, or
initialize it? Maybe the latter, given my comment in
<https://gcc.gnu.org/PR95270>: "make sure to skip/invalidate the
'gcc/gimplify.c:gimplify_scan_omp_clauses' handling"?
Plase add some commentary here in the code, instead of just in the
ChangeLog, something like: "initialize here, so that gimplify doesn't
wrongly do so later" (if that's what it is, and in proper language, of
course).
> --- a/gcc/cp/semantics.c
> +++ b/gcc/cp/semantics.c
> @@ -7334,6 +7334,10 @@ finish_omp_clauses (tree clauses, enum
> c_omp_region_type ort)
> }
> if (cp_oacc_check_attachments (c))
> remove = true;
> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH
> + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH))
> + OMP_CLAUSE_SIZE (c) = size_zero_node;
> break;
> }
> if (t == error_mark_node)
> @@ -7347,6 +7351,10 @@ finish_omp_clauses (tree clauses, enum
> c_omp_region_type ort)
> remove = true;
> break;
> }
> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> + && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ATTACH
> + || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DETACH))
> + OMP_CLAUSE_SIZE (c) = size_zero_node;
> if (REFERENCE_REF_P (t)
> && TREE_CODE (TREE_OPERAND (t, 0)) == COMPONENT_REF)
> {
Likewise.
> --- a/gcc/testsuite/c-c++-common/goacc/mdc-1.c
> +++ b/gcc/testsuite/c-c++-common/goacc/mdc-1.c
Obvious.
In <https://gcc.gnu.org/PR95270> I also requested size vs. bias be
documented in 'include/gomp-constants.h:enum gomp_map_kind'.
Generally, I'm still somewhat confused by the 'bias' usage in libgomp.
Is it really only used for the *initial* attach, but then (correctly so?)
ignored for any later ones? Please add some commentary next to the
respective libgomp code.
Please also include an execution test case, like I had included with
<https://gcc.gnu.org/PR95270>, for example the two files I'm attaching.
Ah actually, since the directive variant now no longer fails, please
merge these into one file, with 'test(bool directive)', and two
'test(false)', 'test(true)' calls from 'main'.
Grüße
Thomas
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander
Walter
#define DIRECTIVE
#include "pr95270_-r.c"
// <https://gcc.gnu.org/PR95270>
#include <assert.h>
#include <openacc.h>
int main()
{
int data;
int *data_p_dev = (int *) acc_create(&data, sizeof data);
int *data_p = &data;
acc_copyin(&data_p, sizeof data_p);
#ifdef DIRECTIVE
# pragma acc enter data attach(data_p)
#else
{
void **ptr = (void **) &data_p;
acc_attach(ptr);
}
#endif
acc_update_self(&data_p, sizeof data_p);
assert (data_p == data_p_dev);
return 0;
}
