| Message ID | 20170228153546.GJ1849@tucnak |
|---|---|
| State | New |
| Headers | show |
On Tue, 28 Feb 2017, Jakub Jelinek wrote: > Hi! > > make_bit_field_ref and its callers are invoked not just during GIMPLE (where > at least fold_truth_andor* isn't really effective; but from what I've tested > it doesn't do a good job in many cases at GENERIC either, because we > fold x.btfld == y.btfld too early into ((BIT_FIELD_REF ^ BIT_FIELD_REF) & cst) == 0 > or similar and fold_truth_andor* can't handle that), but also during GENERIC > and strip away the whole component access path if there are only constant > offsets (various nested COMPONENT_REFs as well as ARRAY_REFs). That is not > really good for constexpr evaluation (we should evaluate constexpr on saved > fn trees before folding, but that is not GCC7 material), which then can't > locate where to look for the value. > > This patch just attempts to reuse as much as possible from orig_inner and > only use adjusted BIT_FIELD_REF on that when in GENERIC (first I've tried to > use instead COMPONENT_REF with DECL_BIT_FIELD_REPRESENTATIVE, but that isn't > handled by constexpr.c either). > Bootstrapped/regtested on x86_64/i686-linux, ok for trunk? Hmm, I'd rather not do anything is_gimple_form specific. Can't we restrict this transform to work on the outermost handled-component only? Ideally we'd want to find a common base of course. The folding is also guarded with optimize != 0 so maybe simply disable it from the FEs? (but yes, you say it does a bad job on GIMPLE which is quite understandable as there's no TRUTH_*, but we can eventually call it from gimplification instead?) Bah, just not really liking these kind of "hacks"... Richaard. > 2017-02-28 Jakub Jelinek <jakub@redhat.com> > > PR c++/79681 > * fold-const.c (make_bit_field_ref): During FE parsing if orig_inner > is COMPONENT_REF, attempt to use its first operand as BIT_FIELD_REF > base. > > * g++.dg/cpp1y/constexpr-79681-1.C: New test. > * g++.dg/cpp1y/constexpr-79681-2.C: New test. > > --- gcc/fold-const.c.jj 2017-02-17 18:29:24.000000000 +0100 > +++ gcc/fold-const.c 2017-02-27 15:05:43.743266260 +0100 > @@ -3862,6 +3862,31 @@ make_bit_field_ref (location_t loc, tree > { > tree result, bftype; > > + /* Attempt not to lose access path if possible during FE folding. */ > + if (!in_gimple_form && TREE_CODE (orig_inner) == COMPONENT_REF) > + { > + tree ninner = TREE_OPERAND (orig_inner, 0); > + machine_mode nmode; > + HOST_WIDE_INT nbitsize, nbitpos; > + tree noffset; > + int nunsignedp, nreversep, nvolatilep = 0; > + tree base = get_inner_reference (ninner, &nbitsize, &nbitpos, > + &noffset, &nmode, &nunsignedp, > + &nreversep, &nvolatilep); > + if (base == inner > + && noffset == NULL_TREE > + && nbitsize >= bitsize > + && nbitpos <= bitpos > + && bitpos + bitsize <= nbitpos + nbitsize > + && !reversep > + && !nreversep > + && !nvolatilep) > + { > + inner = ninner; > + bitpos -= nbitpos; > + } > + } > + > alias_set_type iset = get_alias_set (orig_inner); > if (iset == 0 && get_alias_set (inner) != iset) > inner = fold_build2 (MEM_REF, TREE_TYPE (inner), > --- gcc/testsuite/g++.dg/cpp1y/constexpr-79681-1.C.jj 2017-02-27 15:11:39.177589060 +0100 > +++ gcc/testsuite/g++.dg/cpp1y/constexpr-79681-1.C 2017-02-27 15:11:28.000000000 +0100 > @@ -0,0 +1,17 @@ > +// PR c++/79681 > +// { dg-do compile { target c++14 } } > +// { dg-options "-O2" } > + > +struct A > +{ > + int i : 4; > +}; > + > +constexpr bool > +foo () > +{ > + A x[] = { 1 }; > + return x[0].i; > +} > + > +static_assert (foo(), ""); > --- gcc/testsuite/g++.dg/cpp1y/constexpr-79681-2.C.jj 2017-02-27 15:11:42.252548562 +0100 > +++ gcc/testsuite/g++.dg/cpp1y/constexpr-79681-2.C 2017-02-27 15:10:13.000000000 +0100 > @@ -0,0 +1,39 @@ > +// PR c++/79681 > +// { dg-do compile { target c++14 } } > +// { dg-options "-O2" } > + > +struct A > +{ > + char i : 4; > + char k : 1; > + char l : 3; > +}; > +struct B > +{ > + char j : 4; > +}; > +struct C > +{ > + long long u; > + A a[1]; > + B b[1]; > +}; > + > +constexpr bool > +foo () > +{ > + C c = { 0, { { 5, 0, 2 } }, { { 6 } } }; > + C d = { 0, { { 6, 0, 1 } }, { { 5 } } }; > + return c.a[0].i == d.a[0].i && c.b[0].j == d.b[0].j; > +} > + > +constexpr bool > +bar () > +{ > + C c = { 0, { { 5, 0, 2 } }, { { 6 } } }; > + C d = { 0, { { 6, 0, 1 } }, { { 5 } } }; > + return c.a[0].i == d.a[0].i && c.a[0].l == d.a[0].l; > +} > + > +static_assert (foo () == false, ""); > +static_assert (bar () == false, ""); > > Jakub > >
On Tue, Feb 28, 2017 at 04:43:19PM +0100, Richard Biener wrote: > > This patch just attempts to reuse as much as possible from orig_inner and > > only use adjusted BIT_FIELD_REF on that when in GENERIC (first I've tried to > > use instead COMPONENT_REF with DECL_BIT_FIELD_REPRESENTATIVE, but that isn't > > handled by constexpr.c either). > > Bootstrapped/regtested on x86_64/i686-linux, ok for trunk? > > Hmm, I'd rather not do anything is_gimple_form specific. I think doing what the patch for TREE_CODE (orig_inner) == COMPONENT_REF is probably fine as well, at least assuming say SCCVN uses get_inner_reference or something similar to query to ultimate bases and offsets for CSE purposes. Though, for non-bitfield refs we still use in various places x.foo.bar.baz and in other places MEM_REF[&x + xyz], so I think SCCVN needs to deal with those already anyway. > Can't we > restrict this transform to work on the outermost handled-component only? That is essentially what the patch does; get_inner_reference returns the ultimate base and the patch re-adds the handled-components other than the outermost one back if it yields the same positions/sizes. Perhaps an alternative would be not to call get_inner_reference at all, just something that does handle only the outermost handled component (or add get_inner_reference argument to do that). > Ideally we'd want to find a common base of course. > > The folding is also guarded with optimize != 0 so maybe simply disable > it from the FEs? (but yes, you say it does a bad job on GIMPLE No, I'm afraid we wouldn't fold it at all then. > which is quite understandable as there's no TRUTH_*, but we can > eventually call it from gimplification instead?) The optimize_bit_field_compare perhaps can work in some cases on GIMPLE, the truth_fold_andor* I think would need something like tree-ssa-reassoc.c infrastructure or something similar to gather many comparisons from multiple basic blocks. Jakub
--- gcc/fold-const.c.jj 2017-02-17 18:29:24.000000000 +0100 +++ gcc/fold-const.c 2017-02-27 15:05:43.743266260 +0100 @@ -3862,6 +3862,31 @@ make_bit_field_ref (location_t loc, tree { tree result, bftype; + /* Attempt not to lose access path if possible during FE folding. */ + if (!in_gimple_form && TREE_CODE (orig_inner) == COMPONENT_REF) + { + tree ninner = TREE_OPERAND (orig_inner, 0); + machine_mode nmode; + HOST_WIDE_INT nbitsize, nbitpos; + tree noffset; + int nunsignedp, nreversep, nvolatilep = 0; + tree base = get_inner_reference (ninner, &nbitsize, &nbitpos, + &noffset, &nmode, &nunsignedp, + &nreversep, &nvolatilep); + if (base == inner + && noffset == NULL_TREE + && nbitsize >= bitsize + && nbitpos <= bitpos + && bitpos + bitsize <= nbitpos + nbitsize + && !reversep + && !nreversep + && !nvolatilep) + { + inner = ninner; + bitpos -= nbitpos; + } + } + alias_set_type iset = get_alias_set (orig_inner); if (iset == 0 && get_alias_set (inner) != iset) inner = fold_build2 (MEM_REF, TREE_TYPE (inner), --- gcc/testsuite/g++.dg/cpp1y/constexpr-79681-1.C.jj 2017-02-27 15:11:39.177589060 +0100 +++ gcc/testsuite/g++.dg/cpp1y/constexpr-79681-1.C 2017-02-27 15:11:28.000000000 +0100 @@ -0,0 +1,17 @@ +// PR c++/79681 +// { dg-do compile { target c++14 } } +// { dg-options "-O2" } + +struct A +{ + int i : 4; +}; + +constexpr bool +foo () +{ + A x[] = { 1 }; + return x[0].i; +} + +static_assert (foo(), ""); --- gcc/testsuite/g++.dg/cpp1y/constexpr-79681-2.C.jj 2017-02-27 15:11:42.252548562 +0100 +++ gcc/testsuite/g++.dg/cpp1y/constexpr-79681-2.C 2017-02-27 15:10:13.000000000 +0100 @@ -0,0 +1,39 @@ +// PR c++/79681 +// { dg-do compile { target c++14 } } +// { dg-options "-O2" } + +struct A +{ + char i : 4; + char k : 1; + char l : 3; +}; +struct B +{ + char j : 4; +}; +struct C +{ + long long u; + A a[1]; + B b[1]; +}; + +constexpr bool +foo () +{ + C c = { 0, { { 5, 0, 2 } }, { { 6 } } }; + C d = { 0, { { 6, 0, 1 } }, { { 5 } } }; + return c.a[0].i == d.a[0].i && c.b[0].j == d.b[0].j; +} + +constexpr bool +bar () +{ + C c = { 0, { { 5, 0, 2 } }, { { 6 } } }; + C d = { 0, { { 6, 0, 1 } }, { { 5 } } }; + return c.a[0].i == d.a[0].i && c.a[0].l == d.a[0].l; +} + +static_assert (foo () == false, ""); +static_assert (bar () == false, "");