* [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts @ 2021-05-07 9:02 Richard Biener 2021-05-07 10:21 ` Richard Biener 0 siblings, 1 reply; 6+ messages in thread From: Richard Biener @ 2021-05-07 9:02 UTC (permalink / raw) To: gcc-patches 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. 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. */ + ; 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts 2021-05-07 9:02 [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts Richard Biener @ 2021-05-07 10:21 ` Richard Biener 2021-05-09 4:51 ` Jason Merrill 2021-05-12 17:39 ` Martin Sebor 0 siblings, 2 replies; 6+ messages in thread From: Richard Biener @ 2021-05-07 10:21 UTC (permalink / raw) To: Richard Biener; +Cc: GCC Patches, Jason Merrill 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. 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. */ > + ; > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts 2021-05-07 10:21 ` Richard Biener @ 2021-05-09 4:51 ` Jason Merrill 2021-05-10 9:41 ` Richard Biener 2021-05-12 17:39 ` Martin Sebor 1 sibling, 1 reply; 6+ messages in thread From: Jason Merrill @ 2021-05-09 4:51 UTC (permalink / raw) To: Richard Biener, Richard Biener; +Cc: GCC Patches On 5/7/21 6:21 AM, Richard Biener 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. Hmm, I suppose if the optimization is wrong for PR38615, it's also wrong for the initializer_list variant: extern "C" void abort (void); #include <initializer_list> int f(int t, const int *a) { std::initializer_list<int> b = { 1, 2, 3 }; const int *p = b.begin(); if (!t) return f (1, p); return p == a; } int main(void) { if (f(0, 0)) abort (); return 0; } so adjusting the array-temp testcase seems like the right answer. >> 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. */ >> + ; >> 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts 2021-05-09 4:51 ` Jason Merrill @ 2021-05-10 9:41 ` Richard Biener 0 siblings, 0 replies; 6+ messages in thread From: Richard Biener @ 2021-05-10 9:41 UTC (permalink / raw) To: Jason Merrill; +Cc: Richard Biener, GCC Patches On Sun, 9 May 2021, Jason Merrill wrote: > On 5/7/21 6:21 AM, Richard Biener 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. > > Hmm, I suppose if the optimization is wrong for PR38615, it's also wrong for > the initializer_list variant: > > extern "C" void abort (void); > #include <initializer_list> > > int f(int t, const int *a) > { > std::initializer_list<int> b = { 1, 2, 3 }; > const int *p = b.begin(); > if (!t) > return f (1, p); > return p == a; > } > > int main(void) > { > if (f(0, 0)) > abort (); > return 0; > } > > so adjusting the array-temp testcase seems like the right answer. Done as follows. Bootstrapped / tested on x86_64-unknown-linux-gnu, pushed to trunk sofar. Richard. From a076632e274abe344ca7648b7c7f299273d4cbe0 Mon Sep 17 00:00:00 2001 From: Richard Biener <rguenther@suse.de> Date: Fri, 7 May 2021 09:51:18 +0200 Subject: [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts To: gcc-patches@gcc.gnu.org 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. To not regress g++.dg/tree-ssa/array-temp1.C we have to adjust the testcase to not look for a no longer applied invalid optimization. 2021-05-10 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. * g++.dg/tree-ssa/array-temp1.C: Adjust. --- gcc/cp/call.c | 2 ++ gcc/gimple-fold.c | 4 +++- gcc/testsuite/g++.dg/tree-ssa/array-temp1.C | 6 ------ gcc/testsuite/gcc.dg/pr100464.c | 16 ++++++++++++++++ 4 files changed, 21 insertions(+), 7 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr100464.c diff --git a/gcc/cp/call.c b/gcc/cp/call.c index d985e4e8eda..f07e09a36d1 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. */ + ; else if (TREE_CODE (base) == FUNCTION_DECL) { /* Make sure we create a cgraph node for functions we'll reference. diff --git a/gcc/testsuite/g++.dg/tree-ssa/array-temp1.C b/gcc/testsuite/g++.dg/tree-ssa/array-temp1.C index 97c2e0521c9..3df7aadd30a 100644 --- a/gcc/testsuite/g++.dg/tree-ssa/array-temp1.C +++ b/gcc/testsuite/g++.dg/tree-ssa/array-temp1.C @@ -13,9 +13,3 @@ int f() using AR = const int[]; return AR{ 1,42,3,4,5,6,7,8,9,0 }[5]; } - -int g() -{ - std::initializer_list<int> a = {1,42,3}; - return a.begin()[0]; -} 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts 2021-05-07 10:21 ` Richard Biener 2021-05-09 4:51 ` Jason Merrill @ 2021-05-12 17:39 ` Martin Sebor 2021-05-17 9:22 ` Richard Biener 1 sibling, 1 reply; 6+ messages in thread From: Martin Sebor @ 2021-05-12 17:39 UTC (permalink / raw) To: Richard Biener, Richard Biener; +Cc: GCC Patches 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...) 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts 2021-05-12 17:39 ` Martin Sebor @ 2021-05-17 9:22 ` Richard Biener 0 siblings, 0 replies; 6+ messages in thread From: Richard Biener @ 2021-05-17 9:22 UTC (permalink / raw) To: Martin Sebor; +Cc: Richard Biener, GCC Patches 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) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-17 9:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-07 9:02 [PATCH] middle-end/100464 - avoid spurious TREE_ADDRESSABLE in folding debug stmts Richard Biener 2021-05-07 10:21 ` Richard Biener 2021-05-09 4:51 ` Jason Merrill 2021-05-10 9:41 ` Richard Biener 2021-05-12 17:39 ` Martin Sebor 2021-05-17 9:22 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).