[OpenACC] Bug fix for processing OpenACC data clauses in C++

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.

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, and the tree dumps don't show anything useful.  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?

-Sandra

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 <sandra@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 <sandra@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 <sandra@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 <sandra@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  <ppalka@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

On 3/26/20 8:27 AM, Thomas Schwinge wrote:
> 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 <sandra@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'.  ;-)

I saw that STRIP_NOPS seem to be the preferred way to do things in e.g. 
fold-const.c.  I don't know if there's a reason to use some less general 
form of STRIP_* here?

>> 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?

No.  In the current code on trunk, both places that are being patched 
continue with checks like

TREE_CODE (low_bound) == INTEGER_CST

etc.  So when they're wrapped in NON_LVALUE_EXPRs, it's basically 
failing to detect constants in array dimension specifiers and falling 
through to other code.  (And it's patches to the "other code" that were 
diagnosing the bogus error I saw.)

-Sandra

Hi!

On 2020-03-26T09:09:01-0600, Sandra Loosemore <sandra@codesourcery.com> wrote:
> On 3/26/20 8:27 AM, Thomas Schwinge wrote:
>> 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 <sandra@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'.  ;-)
>
> I saw that STRIP_NOPS seem to be the preferred way to do things in e.g.
> fold-const.c.  I don't know if there's a reason to use some less general
> form of STRIP_* here?
>
>>> 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?
>
> No.  In the current code on trunk, both places that are being patched
> continue with checks like
>
> TREE_CODE (low_bound) == INTEGER_CST
>
> etc.  So when they're wrapped in NON_LVALUE_EXPRs, it's basically
> failing to detect constants in array dimension specifiers and falling
> through to other code.

Eh, indeed...  ;-\ (So we should be able to deduce some misbehavior from
that, to construct a test case.  I'll have a look.)

>>> 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.

While true, that of course doesn't tell us anything what the OMP array
section handling is doing with these.

I guess I was half-asleep when I wrote my email...  ;-/

So.  I'm not objecting to handling 'NON_LVALUE_EXPR's locally here via
any kind of 'STRIP_*', but it somehow doesn't seem the general solution.
Another option seems to be to teach 'fold_simple' to handle
'NON_LVALUE_EXPR's, so that the existing code:

    /* We need to reduce to real constant-values for checks below.  */
    if (length)
      length = fold_simple (length);
    if (low_bound)
      low_bound = fold_simple (low_bound);
    if (low_bound
        && TREE_CODE (low_bound) == INTEGER_CST
        && [...])
      low_bound = fold_convert (sizetype, low_bound);
    if (length
        && TREE_CODE (length) == INTEGER_CST
        && [...])
      length = fold_convert (sizetype, length);

... would then just work.  But: I don't know if 'fold_simple' (and
others?) should handle 'NON_LVALUE_EXPR's, or if there's a reason why it
doesn't.  (Have not yet tried to figure that out.)  What I can tell is
that the attached patch does solve the issue in the C++ OMP array section
handling, and survives a powerpc64le-unknown-linux-gnu
'--enable-checking=yes' (will now re-run with 'fold' checking) bootstrap,
with no testsuite regressions.

Hmm.


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