Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On 12/13/23 11:26, Jakub Jelinek wrote: On Wed, Dec 13, 2023 at 11:24:42AM -0500, Jason Merrill wrote: gcc/testsuite/ChangeLog: * g++.dg/pr112822.C: Require C++17. --- gcc/testsuite/g++.dg/pr112822.C | 1 + 1 file changed, 1 insertion(+) diff --git a/gcc/testsuite/g++.dg/pr112822.C b/gcc/testsuite/g++.dg/pr112822.C index a8557522467..9949fbb08ac 100644 --- a/gcc/testsuite/g++.dg/pr112822.C +++ b/gcc/testsuite/g++.dg/pr112822.C @@ -1,6 +1,7 @@ /* PR tree-optimization/112822 */ /* { dg-do compile { target c++17 } } */ /* { dg-options "-w -O2" } */ +// { dg-do compile { target c++17 } } 2 dg-do compile directives? Oops, I assumed that since my commit still applied on rebase that it hadn't been fixed yet, and didn't see the additional discussion this morning. Reverted. Jason
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On Wed, Dec 13, 2023 at 11:24:42AM -0500, Jason Merrill wrote: > gcc/testsuite/ChangeLog: > > * g++.dg/pr112822.C: Require C++17. > --- > gcc/testsuite/g++.dg/pr112822.C | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/gcc/testsuite/g++.dg/pr112822.C b/gcc/testsuite/g++.dg/pr112822.C > index a8557522467..9949fbb08ac 100644 > --- a/gcc/testsuite/g++.dg/pr112822.C > +++ b/gcc/testsuite/g++.dg/pr112822.C > @@ -1,6 +1,7 @@ > /* PR tree-optimization/112822 */ > /* { dg-do compile { target c++17 } } */ > /* { dg-options "-w -O2" } */ > +// { dg-do compile { target c++17 } } 2 dg-do compile directives? Jakub
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On 12/12/23 21:36, Jason Merrill wrote: On 12/12/23 17:50, Peter Bergner wrote: On 12/12/23 1:26 PM, Richard Biener wrote: Am 12.12.2023 um 19:51 schrieb Peter Bergner : On 12/12/23 12:45 PM, Peter Bergner wrote: +/* PR target/112822 */ Oops, this should be: /* PR tree-optimization/112822 */ It's fixed on my end. Ok Pushed now that Martin has pushed his fix. Thanks! This test is failing for me below C++17, I think you need // { dg-do compile { target c++17 } } or // { dg-require-effective-target c++17 } Fixed thus. From d2b269ce30d77dbfc6c28c75887c330d4698b132 Mon Sep 17 00:00:00 2001 From: Jason Merrill Date: Tue, 12 Dec 2023 21:33:11 -0500 Subject: [PATCH] testsuite: fix g++.dg/pr112822.C To: gcc-patches@gcc.gnu.org gcc/testsuite/ChangeLog: * g++.dg/pr112822.C: Require C++17. --- gcc/testsuite/g++.dg/pr112822.C | 1 + 1 file changed, 1 insertion(+) diff --git a/gcc/testsuite/g++.dg/pr112822.C b/gcc/testsuite/g++.dg/pr112822.C index a8557522467..9949fbb08ac 100644 --- a/gcc/testsuite/g++.dg/pr112822.C +++ b/gcc/testsuite/g++.dg/pr112822.C @@ -1,6 +1,7 @@ /* PR tree-optimization/112822 */ /* { dg-do compile { target c++17 } } */ /* { dg-options "-w -O2" } */ +// { dg-do compile { target c++17 } } /* Verify we do not ICE on the following noisy creduced test case. */ -- 2.39.3
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On 12/13/23 2:05 AM, Jakub Jelinek wrote: > On Wed, Dec 13, 2023 at 08:51:16AM +0100, Richard Biener wrote: >> On Tue, 12 Dec 2023, Peter Bergner wrote: >> >>> On 12/12/23 8:36 PM, Jason Merrill wrote: This test is failing for me below C++17, I think you need // { dg-do compile { target c++17 } } or // { dg-require-effective-target c++17 } >>> >>> Sorry about that. Should we do the above or should we just add >>> -std=c++17 to dg-options? ...or do we need to do both? >> >> Just do the above, the C++ testsuite iterates over all standards, >> adding -std=c++17 would just run that 5 times. But the above >> properly skips unsupported cases. > > I believe if one uses explicit -std=gnu++17 or -std=c++17 in dg-options > then it will not iterate: > # If the testcase specifies a standard, use that one. > # If not, run it under several standards, allowing GNU extensions > # if there's a dg-options line. > if ![search_for $test "-std=*++"] { > and otherwise how many times exactly it iterates depends on what the user > asked for or what effective target is there (normally the default is > to iterate 4 times (98,14,17,20), one can use e.g. > GXX_TESTSUITE_STDS=98,11,14,17,20,23,26 to iterate 7 times, or the > default also changes if c++23, c++26 or c++11_only effective targets > are present somewhere in the test. > > But sure, if the test is valid in C++17, 20, 23, 26, then > // { dg-do compile { target c++17 } } > is best (unless the test is mostly language version independent and > very expensive to compile or run). I confirmed the test case builds with C++17, 20, 23, 26 and errors out with C++11, so I went with your solution. Thanks for the input and sorry for the breakage. Pushed. Peter testsuite: Add dg-do compile target c++17 directive for testcase [PR112822] Add dg-do compile target directive that limits the test case to being built on c++17 compiles or greater. 2023-12-13 Peter Bergner gcc/testsuite/ PR tree-optimization/112822 * g++.dg/pr112822.C: Add dg-do compile target c++17 directive. diff --git a/gcc/testsuite/g++.dg/pr112822.C b/gcc/testsuite/g++.dg/pr112822.C index d1490405493..a8557522467 100644 --- a/gcc/testsuite/g++.dg/pr112822.C +++ b/gcc/testsuite/g++.dg/pr112822.C @@ -1,4 +1,5 @@ /* PR tree-optimization/112822 */ +/* { dg-do compile { target c++17 } } */ /* { dg-options "-w -O2" } */ /* Verify we do not ICE on the following noisy creduced test case. */
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On Wed, Dec 13, 2023 at 08:51:16AM +0100, Richard Biener wrote: > On Tue, 12 Dec 2023, Peter Bergner wrote: > > > On 12/12/23 8:36 PM, Jason Merrill wrote: > > > This test is failing for me below C++17, I think you need > > > > > > // { dg-do compile { target c++17 } } > > > or > > > // { dg-require-effective-target c++17 } > > > > Sorry about that. Should we do the above or should we just add > > -std=c++17 to dg-options? ...or do we need to do both? > > Just do the above, the C++ testsuite iterates over all standards, > adding -std=c++17 would just run that 5 times. But the above > properly skips unsupported cases. I believe if one uses explicit -std=gnu++17 or -std=c++17 in dg-options then it will not iterate: # If the testcase specifies a standard, use that one. # If not, run it under several standards, allowing GNU extensions # if there's a dg-options line. if ![search_for $test "-std=*++"] { and otherwise how many times exactly it iterates depends on what the user asked for or what effective target is there (normally the default is to iterate 4 times (98,14,17,20), one can use e.g. GXX_TESTSUITE_STDS=98,11,14,17,20,23,26 to iterate 7 times, or the default also changes if c++23, c++26 or c++11_only effective targets are present somewhere in the test. But sure, if the test is valid in C++17, 20, 23, 26, then // { dg-do compile { target c++17 } } is best (unless the test is mostly language version independent and very expensive to compile or run). Jakub
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On Tue, 12 Dec 2023, Peter Bergner wrote: > On 12/12/23 8:36 PM, Jason Merrill wrote: > > This test is failing for me below C++17, I think you need > > > > // { dg-do compile { target c++17 } } > > or > > // { dg-require-effective-target c++17 } > > Sorry about that. Should we do the above or should we just add > -std=c++17 to dg-options? ...or do we need to do both? Just do the above, the C++ testsuite iterates over all standards, adding -std=c++17 would just run that 5 times. But the above properly skips unsupported cases. Richard.
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On 12/12/23 8:36 PM, Jason Merrill wrote: > This test is failing for me below C++17, I think you need > > // { dg-do compile { target c++17 } } > or > // { dg-require-effective-target c++17 } Sorry about that. Should we do the above or should we just add -std=c++17 to dg-options? ...or do we need to do both? Peter
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On 12/12/23 17:50, Peter Bergner wrote: On 12/12/23 1:26 PM, Richard Biener wrote: Am 12.12.2023 um 19:51 schrieb Peter Bergner : On 12/12/23 12:45 PM, Peter Bergner wrote: +/* PR target/112822 */ Oops, this should be: /* PR tree-optimization/112822 */ It's fixed on my end. Ok Pushed now that Martin has pushed his fix. Thanks! This test is failing for me below C++17, I think you need // { dg-do compile { target c++17 } } or // { dg-require-effective-target c++17 } Jason
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On 12/12/23 1:26 PM, Richard Biener wrote: >> Am 12.12.2023 um 19:51 schrieb Peter Bergner : >> >> On 12/12/23 12:45 PM, Peter Bergner wrote: >>> +/* PR target/112822 */ >> >> Oops, this should be: >> >> /* PR tree-optimization/112822 */ >> >> It's fixed on my end. > > Ok Pushed now that Martin has pushed his fix. Thanks! Peter
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
> Am 12.12.2023 um 19:51 schrieb Peter Bergner : > > On 12/12/23 12:45 PM, Peter Bergner wrote: >> +/* PR target/112822 */ > > Oops, this should be: > > /* PR tree-optimization/112822 */ > > It's fixed on my end. Ok Richard > Peter > > > >
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On 12/12/23 12:45 PM, Peter Bergner wrote: > +/* PR target/112822 */ Oops, this should be: /* PR tree-optimization/112822 */ It's fixed on my end. Peter
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
On 12/12/23 10:50 AM, Martin Jambor wrote: > The testcase has reasonable size but it is specific to ppc64le and its > altivec vectors. My plan is to ask the bug reporter to massage it into > a target specific testcase in bugzilla. Alternatively I can try to > craft a testcase from scratch but that will take time. I rewrote the Altivec specific part of the testcase to use generic C code and it still ICEs for me on ppc64le using an unpatched compiler. Therefore, I think we can just add the updated testcase to the generic g++ tests. I'll note I was wrong in the bugzilla comments, -O3 -mcpu=power10 is not required to hit the ICE. A simple -O2 on ppc64le is enough to hit the ICE. Ok for trunk? Peter testsuite: Add testcase for already fixed PR [PR112822] gcc/testsuite/ PR tree-optimization/112822 * g++.dg/pr112822.C: New test. diff --git a/gcc/testsuite/g++.dg/pr112822.C b/gcc/testsuite/g++.dg/pr112822.C new file mode 100644 index 000..3921d5c1bbe --- /dev/null +++ b/gcc/testsuite/g++.dg/pr112822.C @@ -0,0 +1,369 @@ +/* PR target/112822 */ +/* { dg-options "-w -O2" } */ + +/* Verify we do not ICE on the following noisy creduced test case. */ + +namespace b { +typedef int c; +template struct d; +template struct d { using f = e; }; +template struct aa; +template struct aa { using f = h; }; +template using ab = typename d::f; +template using n = typename aa::f; +template class af { +public: + typedef __complex__ ah; + template af operator+=(e) { +ah o; +x = o; +return *this; + } + ah x; +}; +} // namespace b +namespace { +enum { p }; +enum { ac, ad }; +struct ae; +struct al; +struct ag; +typedef b::c an; +namespace ai { +template struct ak { typedef aj f; }; +template using ar = typename ak::f; +template struct am { + enum { at }; +}; +template struct ao { + enum { at }; +}; +template struct ap; +template struct aq { + enum { at }; +}; +} // namespace ai +template struct ay; +template class as; +template class ba; +template class aw; +template class be; +template class az; +namespace ai { +template struct bg; +template ::bd> +struct bk; +template struct bf; +template struct bm; +template struct bh; +template ::bj>::at> struct bp { + typedef bi f; +}; +template struct br { + typedef typename bp::f>::f f; +}; +template struct bn; +template struct bn { + typedef aw f; +}; +template struct bx { + typedef typename bn::bs, aj ::bo>::f f; +}; +template struct bt { typedef b::n<0, aj, aj> f; }; +template ::f> struct cb { + enum { bw }; + typedef b::n::f> f; +}; +template ::bs> struct by { + typedef be f; +}; +template struct bz { + typedef typename by::f f; +}; +template struct ch; +template struct ch { typedef ci bd; }; +} // namespace ai +template > struct cg; +template struct cg { typedef aj cn; }; +namespace ai { +template cj cp; +template void cl(bu *cr, cj cs) { ct(cr, cs); } +typedef __attribute__((altivec(vector__))) double co; +void ct(double *cr, co cs) { *(co *)cr = cs; } +struct cq { + co q; +}; +template <> struct bm> { typedef cq f; }; +template <> struct bh { typedef cq bj; }; +void ct(b::af *cr, cq cs) { ct((double *)cr, cs.q); } +template struct cx { + template void cu(cw *a, cj) { +cl(a, cp); + } +}; +} // namespace ai +template class ba : public ay { +public: + typedef ai::ap bu; + typedef b::n::bo, bu, b::n::at, bu, bu>> cv; + typedef ay db; + db::dc; + cv coeff(an dd, an col) const { return dc().coeff(dd, col); } +}; +template class cz : public ba::at> { +public: + ai::ap b; + enum { da, dg, dh, bv, bq, di = dg, bo }; +}; +template class be : public cz { +public: + typedef typename ai::ap::bu bu; + typedef cz db; + db::dc; + template cd &operator+=(const be &); + template az df(de); +}; +template struct ay { + cd &dc() { return *static_cast(this); } + cd dc() const; +}; +template class dl; +namespace ai { +template struct ap> { + typedef bb dj; + typedef bc r; + typedef ap s; + typedef ap t; + typedef typename cg::cn bu; + typedef typename ch::bd>::bd cf; + enum { bo }; +}; +} // namespace ai +template +class az : public dl, ai::ap, ai::bg::bd>> { +public: + typedef dk bb; + typedef Rhs_ bc; + typedef typename ai::bt::f LhsNested; + typedef typename ai::bt::f dn; + typedef ai::ar u; + typedef ai::ar RhsNestedCleaned; + u lhs(); + RhsNestedCleaned rhs(); +}; +template +class dl : public ai::bz, al>::f {}; +namespace ai { +template struct v { typedef ag w; }; +template struct evaluator_traits_base { + typedef typename v::cf>::w w; +}; +template struct ax : evaluator_traits_base {}; +template struct y { static const bool at = false; }; +template class plainobjectbase_evaluator_data { +public: + plainobjectbase_evaluator_data(bu *ptr, an) : data(ptr) {} + an outerStride() { return z; } + bu *data; +}; +template struct evaluator { + typedef cd PlainObjectType; + typedef typename PlainObjectType::bu bu; + enum { IsVectorAtCompileTime }; + enum { Ou
Re: [PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
> Am 12.12.2023 um 17:50 schrieb Martin Jambor : > > Hi, > > PR 112822 revealed a corner case in load_assign_lhs_subreplacements > where it creates invalid gimple: an assignment where on the LHS there > is a complex variable which however is not a gimple register because > it has partial defs and on the right hand side there is a > VIEW_CONVERT_EXPR. This patch invokes force_gimple_operand_gsi on > such statements (like it already does when both sides of a generated > assignment have partial definitions. > > I've made sure the patch passes bootstrap and testsuite on x86_64-linux, > the bug reporter was kind enough to also check the same on an > powerpc64le-linux (see bugzilla comment #8). > > The testcase has reasonable size but it is specific to ppc64le and its > altivec vectors. My plan is to ask the bug reporter to massage it into > a target specific testcase in bugzilla. Alternatively I can try to > craft a testcase from scratch but that will take time. > > Despite the above, is the patch OK for master? Ok Richard > > Thanks, > > Martin > > > > gcc/ChangeLog: > > 2023-12-12 Martin Jambor > >PR tree-optimization/112822 >* tree-sra.cc (load_assign_lhs_subreplacements): Invoke >force_gimple_operand_gsi also when LHS has partial stores and RHS is a >VIEW_CONVERT_EXPR. > --- > gcc/tree-sra.cc | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc > index 3bd0c7a9af0..99a1b0a6d17 100644 > --- a/gcc/tree-sra.cc > +++ b/gcc/tree-sra.cc > @@ -4219,11 +4219,15 @@ load_assign_lhs_subreplacements (struct access *lacc, > if (racc && racc->grp_to_be_replaced) >{ > rhs = get_access_replacement (racc); > + bool vce = false; > if (!useless_type_conversion_p (lacc->type, racc->type)) > -rhs = fold_build1_loc (sad->loc, VIEW_CONVERT_EXPR, > - lacc->type, rhs); > +{ > + rhs = fold_build1_loc (sad->loc, VIEW_CONVERT_EXPR, > + lacc->type, rhs); > + vce = true; > +} > > - if (racc->grp_partial_lhs && lacc->grp_partial_lhs) > + if (lacc->grp_partial_lhs && (vce || racc->grp_partial_lhs)) >rhs = force_gimple_operand_gsi (&sad->old_gsi, rhs, true, >NULL_TREE, true, GSI_SAME_STMT); >} > -- > 2.43.0 >
[PATCH] SRA: Force gimple operand in an additional corner case (PR 112822)
Hi, PR 112822 revealed a corner case in load_assign_lhs_subreplacements where it creates invalid gimple: an assignment where on the LHS there is a complex variable which however is not a gimple register because it has partial defs and on the right hand side there is a VIEW_CONVERT_EXPR. This patch invokes force_gimple_operand_gsi on such statements (like it already does when both sides of a generated assignment have partial definitions. I've made sure the patch passes bootstrap and testsuite on x86_64-linux, the bug reporter was kind enough to also check the same on an powerpc64le-linux (see bugzilla comment #8). The testcase has reasonable size but it is specific to ppc64le and its altivec vectors. My plan is to ask the bug reporter to massage it into a target specific testcase in bugzilla. Alternatively I can try to craft a testcase from scratch but that will take time. Despite the above, is the patch OK for master? Thanks, Martin gcc/ChangeLog: 2023-12-12 Martin Jambor PR tree-optimization/112822 * tree-sra.cc (load_assign_lhs_subreplacements): Invoke force_gimple_operand_gsi also when LHS has partial stores and RHS is a VIEW_CONVERT_EXPR. --- gcc/tree-sra.cc | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc index 3bd0c7a9af0..99a1b0a6d17 100644 --- a/gcc/tree-sra.cc +++ b/gcc/tree-sra.cc @@ -4219,11 +4219,15 @@ load_assign_lhs_subreplacements (struct access *lacc, if (racc && racc->grp_to_be_replaced) { rhs = get_access_replacement (racc); + bool vce = false; if (!useless_type_conversion_p (lacc->type, racc->type)) - rhs = fold_build1_loc (sad->loc, VIEW_CONVERT_EXPR, - lacc->type, rhs); + { + rhs = fold_build1_loc (sad->loc, VIEW_CONVERT_EXPR, +lacc->type, rhs); + vce = true; + } - if (racc->grp_partial_lhs && lacc->grp_partial_lhs) + if (lacc->grp_partial_lhs && (vce || racc->grp_partial_lhs)) rhs = force_gimple_operand_gsi (&sad->old_gsi, rhs, true, NULL_TREE, true, GSI_SAME_STMT); } -- 2.43.0