Hi!
Note that as this code is shared between OpenACC/OpenMP, this might
affect OpenMP, too, as far as I can tell. (Subject updated.) Jakub, can
you please have a look, too?
On 2020-03-25T23:02:38-0600, Sandra Loosemore <san...@codesourcery.com> wrote:
> The attached patch fixes a bug I found in the C++ front end's handling
> of OpenACC data clauses. The problem here is that the C++ front end
> wraps the array bounds expressions in NON_LVALUE_EXPR tree nodes, and
> the code here (which appears to have been copied from similar code in
> the C front end) was failing to strip those before checking to see if
> they were INTEGER_CST nodes, etc.
So, I had a quick look. I'm confirming the 'NON_LVALUE_EXPR' (C++ only,
not seen for C) difference, and that 'STRIP_NOPS' gets rid of these.
However, I also in some code paths see, for example, 'integer_nonzerop'
calls, which internally do 'STRIP_ANY_LOCATION_WRAPPER'.
I don't know if 'STRIP_NOPS' is the correct "hammer" to use here, I don't
know what the usual convention is: explicitly remove (via 'STRIP_NOPS' as
you suggested, or something else), or have the enquiry functions do it
('STRIP_ANY_LOCATION_WRAPPER' as 'integer_nonzerop' is doing, for
example).
Empirical data doesn't mean too much here, of course, I'm not seeing a
lot of explicit 'STRIP_*' calls in 'gcc/cp/semantics.c'. ;-)
> Sadly, I have no test case for this because it was only triggering an
> error in conjunction with some other OpenACC patches that are not yet on
> trunk
So maybe the problem is actually that these "other OpenACC patches" are
not sufficiently sanitizing the input data they're doing checks on?
> and the tree dumps don't show anything useful.
Well, the 'original' tree dumps do show (C++) vs. don't show (C) the
'NON_LVALUE_EXPR's.
> I confirmed that
> the problem exists on trunk without those other patches by examining
> things in the debugger. This is the example I was using for my
> hand-testing:
>
> void f (float a[16][16], float b[16][16], float c[16][16])
> {
> int i, j, k;
>
> #pragma acc kernels copyin(a[0:16][0:16], b[0:16][0:16])
> copyout(c[0:16][0:16])
> {
> for (i = 0; i < 16; i++) {
> for (j = 0; j < 16; j++) {
> float t = 0;
> for (k = 0; k < 16; k++)
> t += a[i][k] * b[k][j];
> c[i][j] = t;
> }
> }
> }
>
> }
>
> Is this patch OK to commit now, or in Stage 1?
First we need to figure out if this is an actual/current bug (which then
needs GCC PR filed, and later also backports to release branches), or
not.
Grüße
Thomas
> commit 3d2cda1e758a54111af04e80ea3dbaa27b3387e7
> Author: Sandra Loosemore <san...@codesourcery.com>
> Date: Wed Mar 25 21:29:17 2020 -0700
>
> Bug fix for processing OpenACC data clauses in C++.
>
> The C++ front end wraps the values in OpenACC data clause array range
> specifications in NON_LVALUE_EXPR tree nodes. The existing code was
> failing to strip these before checking for constant values.
>
> 2020-03-25 Sandra Loosemore <san...@codesourcery.com>
>
> gcc/cp/
> * semantics.c (handle_omp_array_sections_1): Call STRIP_NOPS
> on length and low_bound to unwrap NON_LVALUE_EXPRs.
> (handle_omp_array_sections): Likewise.
>
> diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog
> index 34ccb9f..8945939 100644
> --- a/gcc/cp/ChangeLog
> +++ b/gcc/cp/ChangeLog
> @@ -1,3 +1,9 @@
> +2020-03-25 Sandra Loosemore <san...@codesourcery.com>
> +
> + * semantics.c (handle_omp_array_sections_1): Call STRIP_NOPS
> + on length and low_bound to unwrap NON_LVALUE_EXPRs.
> + (handle_omp_array_sections): Likewise.
> +
> 2020-03-25 Patrick Palka <ppa...@redhat.com>
>
> PR c++/94265
> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> index 2721a55..2efc077 100644
> --- a/gcc/cp/semantics.c
> +++ b/gcc/cp/semantics.c
> @@ -4861,6 +4861,10 @@ handle_omp_array_sections_1 (tree c, tree t, vec<tree>
> &types,
> length = mark_rvalue_use (length);
> /* We need to reduce to real constant-values for checks below. */
> if (length)
> + STRIP_NOPS (length);
> + if (low_bound)
> + STRIP_NOPS (low_bound);
> + if (length)
> length = fold_simple (length);
> if (low_bound)
> low_bound = fold_simple (low_bound);
> @@ -5160,6 +5164,11 @@ handle_omp_array_sections (tree c, enum
> c_omp_region_type ort)
> tree low_bound = TREE_PURPOSE (t);
> tree length = TREE_VALUE (t);
>
> + if (length)
> + STRIP_NOPS (length);
> + if (low_bound)
> + STRIP_NOPS (low_bound);
> +
> i--;
> if (low_bound
> && TREE_CODE (low_bound) == INTEGER_CST
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander
Walter
