Re: [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts

[Richard Biener <rguenther@suse.de>]

On Wed, 12 May 2021, Martin Sebor wrote:

> On 5/7/21 4:21 AM, Richard Biener via Gcc-patches wrote:
> > On Fri, May 7, 2021 at 12:17 PM Richard Biener <rguenther@suse.de> wrote:
> >>
> >> canonicalize_constructor_val was setting TREE_ADDRESSABLE on bases
> >> of ADDR_EXPRs but that's futile when we're dealing with CTOR values
> >> in debug stmts.  This rips out the code which was added for Java
> >> and should have been an assertion when we didn't have debug stmts.
> >>
> >> Bootstrapped and tested on x86_64-unknown-linux-gnu for all languages
> >> which revealed PR100468 for which I added the cp/class.c hunk below.
> >> Re-testing with that in progress.
> >>
> >> OK for trunk and branch?  It looks like this C++ code is new in GCC 11.
> > 
> > I mislooked, the code is old.
> > 
> > This hunk also breaks (or fixes) g++.dg/tree-ssa/array-temp1.C where
> > the gimplifier previously passes the
> > 
> >             && (flag_merge_constants >= 2 || !TREE_ADDRESSABLE (object))
> > 
> > check guarding it against unifying addresses of different instances
> > of variables.  Clearly in the case of the testcase there are addresses to
> > this variable as part of the initializer list construction.  So the hunk
> > fixes
> > wrong-code, but it breaks the testcase.
> > 
> > Any comments?  I can of course change the testcase accordingly.
> 
> Just one belated comment.  I realize you already pushed this change
> but...
> 
> > 
> > Thanks,
> > Richard.
> > 
> >> Thanks,
> >> Richard.
> >>
> >> 2021-05-07  Richard Biener  <rguenther@suse.de>
> >>
> >>          PR middle-end/100464
> >>          PR c++/100468
> >> gcc/
> >>          * gimple-fold.c (canonicalize_constructor_val): Do not set
> >>          TREE_ADDRESSABLE.
> >>
> >> gcc/cp/
> >>          * call.c (set_up_extended_ref_temp): Mark the temporary
> >>          addressable if the TARGET_EXPR was.
> >>
> >> gcc/testsuite/
> >>          * gcc.dg/pr100464.c: New testcase.
> >> ---
> >>   gcc/cp/call.c                   |  2 ++
> >>   gcc/gimple-fold.c               |  4 +++-
> >>   gcc/testsuite/gcc.dg/pr100464.c | 16 ++++++++++++++++
> >>   3 files changed, 21 insertions(+), 1 deletion(-)
> >>   create mode 100644 gcc/testsuite/gcc.dg/pr100464.c
> >>
> >> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> >> index 57bac05fe70..ea97be22f07 100644
> >> --- a/gcc/cp/call.c
> >> +++ b/gcc/cp/call.c
> >> @@ -12478,6 +12478,8 @@ set_up_extended_ref_temp (tree decl, tree expr,
> >> vec<tree, va_gc> **cleanups,
> >>        VAR.  */
> >>     if (TREE_CODE (expr) != TARGET_EXPR)
> >>       expr = get_target_expr (expr);
> >> +  else if (TREE_ADDRESSABLE (expr))
> >> +    TREE_ADDRESSABLE (var) = 1;
> >>
> >>     if (TREE_CODE (decl) == FIELD_DECL
> >>         && extra_warnings && !TREE_NO_WARNING (decl))
> >> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> >> index aa33779b753..768ef89d876 100644
> >> --- a/gcc/gimple-fold.c
> >> +++ b/gcc/gimple-fold.c
> >> @@ -245,7 +245,9 @@ canonicalize_constructor_val (tree cval, tree
> >> from_decl)
> >>         if (TREE_TYPE (base) == error_mark_node)
> >>         return NULL_TREE;
> >>         if (VAR_P (base))
> >> -       TREE_ADDRESSABLE (base) = 1;
> >> +       /* ???  We should be able to assert that TREE_ADDRESSABLE is set,
> >> +          but since the use can be in a debug stmt we can't.  */
> >> +       ;
> 
> ...as I mentioned before, I find these question marks confusing
> (and there are over a thousand instances of them in GCC sources).
> They bring the comment that follows into question.  Please consider
> spelling out in words what you mean instead.  (E.g., use FIXME: We
> should be able to assert...)

To me ??? is a synonym to FIXME.  The vectorizer has TODO.

There are 856 ??? comments in gcc/*.c and only 319 FIXME ones, so
it seems ??? is prefered.

I find ??? less confusing - FIXME is sth that needs fixing while
??? reads to me as some "this could be done better but isn't a bug".

Anyway, just my personal preference and reading of course.

Richard.

> Martin
> 
> >>         else if (TREE_CODE (base) == FUNCTION_DECL)
> >>          {
> >>            /* Make sure we create a cgraph node for functions we'll
> >> reference.
> >> diff --git a/gcc/testsuite/gcc.dg/pr100464.c
> >> b/gcc/testsuite/gcc.dg/pr100464.c
> >> new file mode 100644
> >> index 00000000000..46cc37dff54
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/pr100464.c
> >> @@ -0,0 +1,16 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O3 -fcompare-debug" } */
> >> +
> >> +int *a;
> >> +static int b, c, d, e, g, h;
> >> +int f;
> >> +void i() {
> >> +  int *j[] = {&e, &b, &b, &d, &b, &b, &g, &e, &g, &b, &b,
> >> +              &b, &b, &g, &e, &e, &b, &b, &d, &b, &b, &e,
> >> +              &e, &g, &b, &b, &b, &b, &g, &e, &g, &c, &e};
> >> +  int **k = &j[5];
> >> +  for (; f;)
> >> +    b |= *a;
> >> +  *k = &h;
> >> +}
> >> +int main() {}
> >> --
> >> 2.26.2
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)