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 <rguent...@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 <rguent...@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 <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
