| Message ID | 5d9c026a-12a7-d6f7-e1d7-da0382ac8f2a@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 23, 2017 at 6:49 AM, Jeff Law <law@redhat.com> wrote: > > tree-ssa-dse.c needs to verify when two writes have the same base address. > Right now it uses pointer equality. The testcase in BZ79578 shows that we > should have been using operand_equal_p. > > This one-liner fixes that oversight. Bootstrapped and regression tested on > x86_64-linux-gnu. Installed on the trunk. > > Jeff > > commit ef506ec9114a7fe27d9ee892c17edd100f72a963 > Author: law <law@138bc75d-0d04-0410-961f-82ee72b054a4> > Date: Thu Feb 23 05:47:43 2017 +0000 > > PR tree-optimization/79578 > * tree-ssa-dse.c (clear_bytes_written_by): Use operand_equal_p > to compare base operands. > > PR tree-optimization/79578 > * g++.dg/tree-ssa/ssa-dse-3.C: New test. > > git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@245675 > 138bc75d-0d04-0410-961f-82ee72b054a4 > > diff --git a/gcc/ChangeLog b/gcc/ChangeLog > index 7155850..6da1d74 100644 > --- a/gcc/ChangeLog > +++ b/gcc/ChangeLog > @@ -1,3 +1,9 @@ > +2017-02-22 Jeff Law <law@redhat.com> > + > + PR tree-optimization/79578 > + * tree-ssa-dse.c (clear_bytes_written_by): Use operand_equal_p > + to compare base operands. > + > 2017-02-22 Segher Boessenkool <segher@kernel.crashing.org> > > PR target/79211 > diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog > index ea5e251..d900cc3 100644 > --- a/gcc/testsuite/ChangeLog > +++ b/gcc/testsuite/ChangeLog > @@ -1,3 +1,8 @@ > +2017-02-22 Jeff Law <law@redhat.com> > + > + PR tree-optimization/79578 > + * g++.dg/tree-ssa/ssa-dse-3.C: New test. > + > 2017-02-22 Sameera Deshpande <sameera.deshpande@imgtec.com> > > * gcc.target/mips/msa-fp-cc.c: New test. > diff --git a/gcc/testsuite/g++.dg/tree-ssa/ssa-dse-3.C > b/gcc/testsuite/g++.dg/tree-ssa/ssa-dse-3.C > new file mode 100644 > index 0000000..fe8f309 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/tree-ssa/ssa-dse-3.C > @@ -0,0 +1,28 @@ > +/* { dg-do compile } */ > +/* { dg-options "-std=c++14 -O3 -fdump-tree-dse1-details" } */ > + > +#include <new> > +#include <cstdint> > + > +struct A > +{ > + std::uint16_t a, b; > +}; > + > +A* f(char* b) __attribute__((noinline)); > + > +A* f(char* b) { > + auto a = new(b) A{}; > + a->a = 1; > + a->b = 2; > + return a; > +} > + > +int main() { > + char b[sizeof(A)] alignas(A); > + f(b); > +} > + > + > +/* { dg-final { scan-tree-dump "Deleted dead store: " "dse1" } } */ > + > diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c > index 84c0b11..a82e164 100644 > --- a/gcc/tree-ssa-dse.c > +++ b/gcc/tree-ssa-dse.c > @@ -176,7 +176,7 @@ clear_bytes_written_by (sbitmap live_bytes, gimple > *stmt, ao_ref *ref) > /* Verify we have the same base memory address, the write > has a known size and overlaps with REF. */ > if (valid_ao_ref_for_dse (&write) > - && write.base == ref->base > + && operand_equal_p (write.base, ref->base, 0) As you've identified size and offset match you are really interested in comparing the base addresses and thus should use OEP_ADDRESS_OF. Richard. > && write.size == write.max_size > && ((write.offset < ref->offset > && write.offset + write.size > ref->offset) >
On 02/23/2017 02:02 AM, Richard Biener wrote: >> diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c >> index 84c0b11..a82e164 100644 >> --- a/gcc/tree-ssa-dse.c >> +++ b/gcc/tree-ssa-dse.c >> @@ -176,7 +176,7 @@ clear_bytes_written_by (sbitmap live_bytes, gimple >> *stmt, ao_ref *ref) >> /* Verify we have the same base memory address, the write >> has a known size and overlaps with REF. */ >> if (valid_ao_ref_for_dse (&write) >> - && write.base == ref->base >> + && operand_equal_p (write.base, ref->base, 0) > > As you've identified size and offset match you are really interested > in comparing the base addresses and thus should use OEP_ADDRESS_OF. I pondered that, but (perhaps incorrectly) thought that OEP_ADDRESS_OF was an optimization and that a more simple o_e_p with no flags was safer. I'm happy to change it, particularly if it's a correctness issue (in which case I think we've designed a horrible API for o_e_p, but such is life). In fact, I've already bootstrapped and regression tested that change. jeff
On Thu, Feb 23, 2017 at 10:06 PM, Jeff Law <law@redhat.com> wrote: > On 02/23/2017 02:02 AM, Richard Biener wrote: > >>> diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c >>> index 84c0b11..a82e164 100644 >>> --- a/gcc/tree-ssa-dse.c >>> +++ b/gcc/tree-ssa-dse.c >>> @@ -176,7 +176,7 @@ clear_bytes_written_by (sbitmap live_bytes, gimple >>> *stmt, ao_ref *ref) >>> /* Verify we have the same base memory address, the write >>> has a known size and overlaps with REF. */ >>> if (valid_ao_ref_for_dse (&write) >>> - && write.base == ref->base >>> + && operand_equal_p (write.base, ref->base, 0) >> >> >> As you've identified size and offset match you are really interested >> in comparing the base addresses and thus should use OEP_ADDRESS_OF. > > I pondered that, but (perhaps incorrectly) thought that OEP_ADDRESS_OF was > an optimization and that a more simple o_e_p with no flags was safer. > > I'm happy to change it, particularly if it's a correctness issue (in which > case I think we've designed a horrible API for o_e_p, but such is life). In > fact, I've already bootstrapped and regression tested that change. It's indeed an optimization to use OEP_ADDRESS_OF and 0 is more conservative. Richard. > jeff >
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 7155850..6da1d74 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,9 @@ +2017-02-22 Jeff Law <law@redhat.com> + + PR tree-optimization/79578 + * tree-ssa-dse.c (clear_bytes_written_by): Use operand_equal_p + to compare base operands. + 2017-02-22 Segher Boessenkool <segher@kernel.crashing.org> PR target/79211 diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index ea5e251..d900cc3 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2017-02-22 Jeff Law <law@redhat.com> + + PR tree-optimization/79578 + * g++.dg/tree-ssa/ssa-dse-3.C: New test. + 2017-02-22 Sameera Deshpande <sameera.deshpande@imgtec.com> * gcc.target/mips/msa-fp-cc.c: New test. diff --git a/gcc/testsuite/g++.dg/tree-ssa/ssa-dse-3.C b/gcc/testsuite/g++.dg/tree-ssa/ssa-dse-3.C new file mode 100644 index 0000000..fe8f309 --- /dev/null +++ b/gcc/testsuite/g++.dg/tree-ssa/ssa-dse-3.C @@ -0,0 +1,28 @@ +/* { dg-do compile } */ +/* { dg-options "-std=c++14 -O3 -fdump-tree-dse1-details" } */ + +#include <new> +#include <cstdint> + +struct A +{ + std::uint16_t a, b; +}; + +A* f(char* b) __attribute__((noinline)); + +A* f(char* b) { + auto a = new(b) A{}; + a->a = 1; + a->b = 2; + return a; +} + +int main() { + char b[sizeof(A)] alignas(A); + f(b); +} + + +/* { dg-final { scan-tree-dump "Deleted dead store: " "dse1" } } */ + diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c index 84c0b11..a82e164 100644 --- a/gcc/tree-ssa-dse.c +++ b/gcc/tree-ssa-dse.c @@ -176,7 +176,7 @@ clear_bytes_written_by (sbitmap live_bytes, gimple *stmt, ao_ref *ref) /* Verify we have the same base memory address, the write has a known size and overlaps with REF. */ if (valid_ao_ref_for_dse (&write) - && write.base == ref->base + && operand_equal_p (write.base, ref->base, 0) && write.size == write.max_size && ((write.offset < ref->offset && write.offset + write.size > ref->offset)
tree-ssa-dse.c needs to verify when two writes have the same base address. Right now it uses pointer equality. The testcase in BZ79578 shows that we should have been using operand_equal_p. This one-liner fixes that oversight. Bootstrapped and regression tested on x86_64-linux-gnu. Installed on the trunk. Jeff commit ef506ec9114a7fe27d9ee892c17edd100f72a963 Author: law <law@138bc75d-0d04-0410-961f-82ee72b054a4> Date: Thu Feb 23 05:47:43 2017 +0000 PR tree-optimization/79578 * tree-ssa-dse.c (clear_bytes_written_by): Use operand_equal_p to compare base operands. PR tree-optimization/79578 * g++.dg/tree-ssa/ssa-dse-3.C: New test. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@245675 138bc75d-0d04-0410-961f-82ee72b054a4