[WIP] Fold 'NON_LVALUE_EXPR' some more (was: C++ 'NON_LVALUE_EXPR' in OMP array section handling)

Message ID yxfpd06sb7w0.fsf@hertz.schwinge.homeip.net
State New
Headers show
Series
  • [WIP] Fold 'NON_LVALUE_EXPR' some more (was: C++ 'NON_LVALUE_EXPR' in OMP array section handling)
Related show

Commit Message

Thomas Schwinge May 25, 2020, 10:56 a.m.
Hi!

Anyone have any input here, especially whether something like the WIP
patch attached to generally "Fold 'NON_LVALUE_EXPR' some more" is
preferable over local 'STRIP_NOPS'?

On 2020-03-26T20:53:19+0100, I wrote:
> 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


>> In the current code [we have]

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


(Have not yet been able to look into constructing any test cases.)


> 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

Patch

From 611fbe24b7e459829c0a304a58963d4987c8de0a Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Thu, 26 Mar 2020 21:22:54 +0100
Subject: [PATCH] Fold 'NON_LVALUE_EXPR' some more

---
 gcc/cp/constexpr.c | 1 +
 gcc/fold-const.c   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 192face9a3a..f31d61c1460 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -6650,6 +6650,7 @@  fold_simple_1 (tree t)
     case BIT_NOT_EXPR:
     case TRUTH_NOT_EXPR:
     case NOP_EXPR:
+    case NON_LVALUE_EXPR:
     case VIEW_CONVERT_EXPR:
     case CONVERT_EXPR:
     case FLOAT_EXPR:
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 71a1d3eb735..b6bc5080ff3 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -1739,6 +1739,7 @@  const_unop (enum tree_code code, tree type, tree arg0)
   switch (code)
     {
     CASE_CONVERT:
+    case NON_LVALUE_EXPR:
     case FLOAT_EXPR:
     case FIX_TRUNC_EXPR:
     case FIXED_CONVERT_EXPR:
-- 
2.17.1