| Message ID | CAAe5K+U9C1_WUE=OA-1goTvUFSTOTeHoAyzzcfMyoDjDywEzPw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 13, 2014 at 3:20 PM, Teresa Johnson <tejohnson@google.com> wrote: > Here is the new patch. Bootstrapped and tested on > x86_64-unknown-linux-gnu. OK for trunk? Ok for trunk and branches. Thanks, Richard. > Thanks, > Teresa > > 2014-11-13 <tejohnson@google.com> > > gcc: > PR tree-optimization/63841 > * tree.c (initializer_zerop): A constructor with no elements > does not zero initialize. > > gcc/testsuite: > PR tree-optimization/63841 > * g++.dg/tree-ssa/pr63841.C: New test. > > Index: tree.c > =================================================================== > --- tree.c (revision 217190) > +++ tree.c (working copy) > @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) > { > unsigned HOST_WIDE_INT idx; > > + if (TREE_CLOBBER_P (init)) > + return false; > FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) > if (!initializer_zerop (elt)) > return false; > Index: testsuite/g++.dg/tree-ssa/pr63841.C > =================================================================== > --- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0) > +++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy) > @@ -0,0 +1,38 @@ > +/* { dg-do run } */ > +/* { dg-options "-O2" } */ > + > +#include <cstdio> > +#include <string> > + > +std::string __attribute__ ((noinline)) comp_test_write() { > + std::string data; > + > + for (int i = 0; i < 2; ++i) { > + char b = 1 >> (i * 8); > + data.append(&b, 1); > + } > + > + return data; > +} > + > +std::string __attribute__ ((noinline)) comp_test_write_good() { > + std::string data; > + > + char b; > + for (int i = 0; i < 2; ++i) { > + b = 1 >> (i * 8); > + data.append(&b, 1); > + } > + > + return data; > +} > + > +int main() { > + std::string good = comp_test_write_good(); > + printf("expected: %hx\n", *(short*)good.c_str()); > + > + std::string bad = comp_test_write(); > + printf("got: %hx\n", *(short*)bad.c_str()); > + > + return good != bad; > +} > > On Wed, Nov 12, 2014 at 9:40 PM, Andrew Pinski <pinskia@gmail.com> wrote: >> On Wed, Nov 12, 2014 at 9:38 PM, Teresa Johnson <tejohnson@google.com> wrote: >>> On Wed, Nov 12, 2014 at 9:30 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>>> On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>>> Added testcase. Here is the new patch: >>>>> >>>>> 2014-11-12 <tejohnson@google.com> >>>>> >>>>> gcc: >>>>> PR tree-optimization/63841 >>>>> * tree.c (initializer_zerop): A constructor with no elements >>>>> does not zero initialize. >>>> >>>> Actually an empty constructor does zero initialize. A clobber does not. >>> >>> Ok, thanks, I wasn't sure. >>> >>>> >>>> The check you want is TREE_CLOBBER_P instead. >>> >>> So in initializer_zerop, a constructor that is a TREE_CLOBBER_P would >>> return false, right? I will make that change and retest. >> >> Yes that is correct. Note TREE_CLOBBER_P already checks if it is an >> constructor. >> >> Thanks, >> Andrew >> >>> >>> Thanks, >>> Teresa >>> >>>> >>>> Thanks, >>>> Andrew >>>> >>>> >>>>> >>>>> gcc/testsuite: >>>>> * g++.dg/tree-ssa/pr63841.C: New test. >>>>> >>>>> Index: tree.c >>>>> =================================================================== >>>>> --- tree.c (revision 217190) >>>>> +++ tree.c (working copy) >>>>> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >>>>> { >>>>> unsigned HOST_WIDE_INT idx; >>>>> >>>>> + if (!CONSTRUCTOR_NELTS (init)) >>>>> + return false; >>>>> FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) >>>>> if (!initializer_zerop (elt)) >>>>> return false; >>>>> Index: testsuite/g++.dg/tree-ssa/pr63841.C >>>>> =================================================================== >>>>> --- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0) >>>>> +++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy) >>>>> @@ -0,0 +1,38 @@ >>>>> +/* { dg-do run } */ >>>>> +/* { dg-options "-O2" } */ >>>>> + >>>>> +#include <cstdio> >>>>> +#include <string> >>>>> + >>>>> +std::string __attribute__ ((noinline)) comp_test_write() { >>>>> + std::string data; >>>>> + >>>>> + for (int i = 0; i < 2; ++i) { >>>>> + char b = 1 >> (i * 8); >>>>> + data.append(&b, 1); >>>>> + } >>>>> + >>>>> + return data; >>>>> +} >>>>> + >>>>> +std::string __attribute__ ((noinline)) comp_test_write_good() { >>>>> + std::string data; >>>>> + >>>>> + char b; >>>>> + for (int i = 0; i < 2; ++i) { >>>>> + b = 1 >> (i * 8); >>>>> + data.append(&b, 1); >>>>> + } >>>>> + >>>>> + return data; >>>>> +} >>>>> + >>>>> +int main() { >>>>> + std::string good = comp_test_write_good(); >>>>> + printf("expected: %hx\n", *(short*)good.c_str()); >>>>> + >>>>> + std::string bad = comp_test_write(); >>>>> + printf("got: %hx\n", *(short*)bad.c_str()); >>>>> + >>>>> + return good != bad; >>>>> +} >>>>> >>>>> On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li <davidxl@google.com> wrote: >>>>>> missing test case? >>>>>> >>>>>> David >>>>>> >>>>>> On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>>>>> This patch fixes an issue where tree-strlen was incorrectly removing a >>>>>>> store of 0 into a string because it thought a prior CLOBBER (which is >>>>>>> an empty constructor with no elements) was zero-initializing the >>>>>>> string. >>>>>>> >>>>>>> Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? >>>>>>> >>>>>>> Thanks, >>>>>>> Teresa >>>>>>> >>>>>>> 2014-11-12 <tejohnson@google.com> >>>>>>> >>>>>>> PR tree-optimization/63841 >>>>>>> * tree.c (initializer_zerop): A constructor with no elements >>>>>>> does not zero initialize. >>>>>>> >>>>>>> Index: tree.c >>>>>>> =================================================================== >>>>>>> --- tree.c (revision 217190) >>>>>>> +++ tree.c (working copy) >>>>>>> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >>>>>>> { >>>>>>> unsigned HOST_WIDE_INT idx; >>>>>>> >>>>>>> + if (!CONSTRUCTOR_NELTS (init)) >>>>>>> + return false; >>>>>>> FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) >>>>>>> if (!initializer_zerop (elt)) >>>>>>> return false; >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >>>>> >>>>> >>>>> >>>>> -- >>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >>> >>> >>> >>> -- >>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
On Thu, Nov 13, 2014 at 3:32 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Nov 13, 2014 at 3:20 PM, Teresa Johnson <tejohnson@google.com> wrote: >> Here is the new patch. Bootstrapped and tested on >> x86_64-unknown-linux-gnu. OK for trunk? > > Ok for trunk and branches. Err - please fix the changelog entry wording to "A clobber does not zero inintiaize" > Thanks, > Richard. > >> Thanks, >> Teresa >> >> 2014-11-13 <tejohnson@google.com> >> >> gcc: >> PR tree-optimization/63841 >> * tree.c (initializer_zerop): A constructor with no elements >> does not zero initialize. >> >> gcc/testsuite: >> PR tree-optimization/63841 >> * g++.dg/tree-ssa/pr63841.C: New test. >> >> Index: tree.c >> =================================================================== >> --- tree.c (revision 217190) >> +++ tree.c (working copy) >> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >> { >> unsigned HOST_WIDE_INT idx; >> >> + if (TREE_CLOBBER_P (init)) >> + return false; >> FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) >> if (!initializer_zerop (elt)) >> return false; >> Index: testsuite/g++.dg/tree-ssa/pr63841.C >> =================================================================== >> --- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0) >> +++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy) >> @@ -0,0 +1,38 @@ >> +/* { dg-do run } */ >> +/* { dg-options "-O2" } */ >> + >> +#include <cstdio> >> +#include <string> >> + >> +std::string __attribute__ ((noinline)) comp_test_write() { >> + std::string data; >> + >> + for (int i = 0; i < 2; ++i) { >> + char b = 1 >> (i * 8); >> + data.append(&b, 1); >> + } >> + >> + return data; >> +} >> + >> +std::string __attribute__ ((noinline)) comp_test_write_good() { >> + std::string data; >> + >> + char b; >> + for (int i = 0; i < 2; ++i) { >> + b = 1 >> (i * 8); >> + data.append(&b, 1); >> + } >> + >> + return data; >> +} >> + >> +int main() { >> + std::string good = comp_test_write_good(); >> + printf("expected: %hx\n", *(short*)good.c_str()); >> + >> + std::string bad = comp_test_write(); >> + printf("got: %hx\n", *(short*)bad.c_str()); >> + >> + return good != bad; >> +} >> >> On Wed, Nov 12, 2014 at 9:40 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>> On Wed, Nov 12, 2014 at 9:38 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>> On Wed, Nov 12, 2014 at 9:30 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>>>> On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>>>> Added testcase. Here is the new patch: >>>>>> >>>>>> 2014-11-12 <tejohnson@google.com> >>>>>> >>>>>> gcc: >>>>>> PR tree-optimization/63841 >>>>>> * tree.c (initializer_zerop): A constructor with no elements >>>>>> does not zero initialize. >>>>> >>>>> Actually an empty constructor does zero initialize. A clobber does not. >>>> >>>> Ok, thanks, I wasn't sure. >>>> >>>>> >>>>> The check you want is TREE_CLOBBER_P instead. >>>> >>>> So in initializer_zerop, a constructor that is a TREE_CLOBBER_P would >>>> return false, right? I will make that change and retest. >>> >>> Yes that is correct. Note TREE_CLOBBER_P already checks if it is an >>> constructor. >>> >>> Thanks, >>> Andrew >>> >>>> >>>> Thanks, >>>> Teresa >>>> >>>>> >>>>> Thanks, >>>>> Andrew >>>>> >>>>> >>>>>> >>>>>> gcc/testsuite: >>>>>> * g++.dg/tree-ssa/pr63841.C: New test. >>>>>> >>>>>> Index: tree.c >>>>>> =================================================================== >>>>>> --- tree.c (revision 217190) >>>>>> +++ tree.c (working copy) >>>>>> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >>>>>> { >>>>>> unsigned HOST_WIDE_INT idx; >>>>>> >>>>>> + if (!CONSTRUCTOR_NELTS (init)) >>>>>> + return false; >>>>>> FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) >>>>>> if (!initializer_zerop (elt)) >>>>>> return false; >>>>>> Index: testsuite/g++.dg/tree-ssa/pr63841.C >>>>>> =================================================================== >>>>>> --- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0) >>>>>> +++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy) >>>>>> @@ -0,0 +1,38 @@ >>>>>> +/* { dg-do run } */ >>>>>> +/* { dg-options "-O2" } */ >>>>>> + >>>>>> +#include <cstdio> >>>>>> +#include <string> >>>>>> + >>>>>> +std::string __attribute__ ((noinline)) comp_test_write() { >>>>>> + std::string data; >>>>>> + >>>>>> + for (int i = 0; i < 2; ++i) { >>>>>> + char b = 1 >> (i * 8); >>>>>> + data.append(&b, 1); >>>>>> + } >>>>>> + >>>>>> + return data; >>>>>> +} >>>>>> + >>>>>> +std::string __attribute__ ((noinline)) comp_test_write_good() { >>>>>> + std::string data; >>>>>> + >>>>>> + char b; >>>>>> + for (int i = 0; i < 2; ++i) { >>>>>> + b = 1 >> (i * 8); >>>>>> + data.append(&b, 1); >>>>>> + } >>>>>> + >>>>>> + return data; >>>>>> +} >>>>>> + >>>>>> +int main() { >>>>>> + std::string good = comp_test_write_good(); >>>>>> + printf("expected: %hx\n", *(short*)good.c_str()); >>>>>> + >>>>>> + std::string bad = comp_test_write(); >>>>>> + printf("got: %hx\n", *(short*)bad.c_str()); >>>>>> + >>>>>> + return good != bad; >>>>>> +} >>>>>> >>>>>> On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li <davidxl@google.com> wrote: >>>>>>> missing test case? >>>>>>> >>>>>>> David >>>>>>> >>>>>>> On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>>>>>> This patch fixes an issue where tree-strlen was incorrectly removing a >>>>>>>> store of 0 into a string because it thought a prior CLOBBER (which is >>>>>>>> an empty constructor with no elements) was zero-initializing the >>>>>>>> string. >>>>>>>> >>>>>>>> Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Teresa >>>>>>>> >>>>>>>> 2014-11-12 <tejohnson@google.com> >>>>>>>> >>>>>>>> PR tree-optimization/63841 >>>>>>>> * tree.c (initializer_zerop): A constructor with no elements >>>>>>>> does not zero initialize. >>>>>>>> >>>>>>>> Index: tree.c >>>>>>>> =================================================================== >>>>>>>> --- tree.c (revision 217190) >>>>>>>> +++ tree.c (working copy) >>>>>>>> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >>>>>>>> { >>>>>>>> unsigned HOST_WIDE_INT idx; >>>>>>>> >>>>>>>> + if (!CONSTRUCTOR_NELTS (init)) >>>>>>>> + return false; >>>>>>>> FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) >>>>>>>> if (!initializer_zerop (elt)) >>>>>>>> return false; >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >>>> >>>> >>>> >>>> -- >>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >> >> >> >> -- >> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
On Thu, Nov 13, 2014 at 6:32 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Nov 13, 2014 at 3:32 PM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Thu, Nov 13, 2014 at 3:20 PM, Teresa Johnson <tejohnson@google.com> wrote: >>> Here is the new patch. Bootstrapped and tested on >>> x86_64-unknown-linux-gnu. OK for trunk? >> >> Ok for trunk and branches. > > Err - please fix the changelog entry wording to "A clobber does > not zero inintiaize" Thanks for catching that - copied from the original patch. Will commit to trunk now then to gcc-4_9 after regression testing with the branch. Thanks, Teresa > >> Thanks, >> Richard. >> >>> Thanks, >>> Teresa >>> >>> 2014-11-13 <tejohnson@google.com> >>> >>> gcc: >>> PR tree-optimization/63841 >>> * tree.c (initializer_zerop): A constructor with no elements >>> does not zero initialize. >>> >>> gcc/testsuite: >>> PR tree-optimization/63841 >>> * g++.dg/tree-ssa/pr63841.C: New test. >>> >>> Index: tree.c >>> =================================================================== >>> --- tree.c (revision 217190) >>> +++ tree.c (working copy) >>> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >>> { >>> unsigned HOST_WIDE_INT idx; >>> >>> + if (TREE_CLOBBER_P (init)) >>> + return false; >>> FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) >>> if (!initializer_zerop (elt)) >>> return false; >>> Index: testsuite/g++.dg/tree-ssa/pr63841.C >>> =================================================================== >>> --- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0) >>> +++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy) >>> @@ -0,0 +1,38 @@ >>> +/* { dg-do run } */ >>> +/* { dg-options "-O2" } */ >>> + >>> +#include <cstdio> >>> +#include <string> >>> + >>> +std::string __attribute__ ((noinline)) comp_test_write() { >>> + std::string data; >>> + >>> + for (int i = 0; i < 2; ++i) { >>> + char b = 1 >> (i * 8); >>> + data.append(&b, 1); >>> + } >>> + >>> + return data; >>> +} >>> + >>> +std::string __attribute__ ((noinline)) comp_test_write_good() { >>> + std::string data; >>> + >>> + char b; >>> + for (int i = 0; i < 2; ++i) { >>> + b = 1 >> (i * 8); >>> + data.append(&b, 1); >>> + } >>> + >>> + return data; >>> +} >>> + >>> +int main() { >>> + std::string good = comp_test_write_good(); >>> + printf("expected: %hx\n", *(short*)good.c_str()); >>> + >>> + std::string bad = comp_test_write(); >>> + printf("got: %hx\n", *(short*)bad.c_str()); >>> + >>> + return good != bad; >>> +} >>> >>> On Wed, Nov 12, 2014 at 9:40 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>>> On Wed, Nov 12, 2014 at 9:38 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>>> On Wed, Nov 12, 2014 at 9:30 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>>>>> On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>>>>> Added testcase. Here is the new patch: >>>>>>> >>>>>>> 2014-11-12 <tejohnson@google.com> >>>>>>> >>>>>>> gcc: >>>>>>> PR tree-optimization/63841 >>>>>>> * tree.c (initializer_zerop): A constructor with no elements >>>>>>> does not zero initialize. >>>>>> >>>>>> Actually an empty constructor does zero initialize. A clobber does not. >>>>> >>>>> Ok, thanks, I wasn't sure. >>>>> >>>>>> >>>>>> The check you want is TREE_CLOBBER_P instead. >>>>> >>>>> So in initializer_zerop, a constructor that is a TREE_CLOBBER_P would >>>>> return false, right? I will make that change and retest. >>>> >>>> Yes that is correct. Note TREE_CLOBBER_P already checks if it is an >>>> constructor. >>>> >>>> Thanks, >>>> Andrew >>>> >>>>> >>>>> Thanks, >>>>> Teresa >>>>> >>>>>> >>>>>> Thanks, >>>>>> Andrew >>>>>> >>>>>> >>>>>>> >>>>>>> gcc/testsuite: >>>>>>> * g++.dg/tree-ssa/pr63841.C: New test. >>>>>>> >>>>>>> Index: tree.c >>>>>>> =================================================================== >>>>>>> --- tree.c (revision 217190) >>>>>>> +++ tree.c (working copy) >>>>>>> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >>>>>>> { >>>>>>> unsigned HOST_WIDE_INT idx; >>>>>>> >>>>>>> + if (!CONSTRUCTOR_NELTS (init)) >>>>>>> + return false; >>>>>>> FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) >>>>>>> if (!initializer_zerop (elt)) >>>>>>> return false; >>>>>>> Index: testsuite/g++.dg/tree-ssa/pr63841.C >>>>>>> =================================================================== >>>>>>> --- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0) >>>>>>> +++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy) >>>>>>> @@ -0,0 +1,38 @@ >>>>>>> +/* { dg-do run } */ >>>>>>> +/* { dg-options "-O2" } */ >>>>>>> + >>>>>>> +#include <cstdio> >>>>>>> +#include <string> >>>>>>> + >>>>>>> +std::string __attribute__ ((noinline)) comp_test_write() { >>>>>>> + std::string data; >>>>>>> + >>>>>>> + for (int i = 0; i < 2; ++i) { >>>>>>> + char b = 1 >> (i * 8); >>>>>>> + data.append(&b, 1); >>>>>>> + } >>>>>>> + >>>>>>> + return data; >>>>>>> +} >>>>>>> + >>>>>>> +std::string __attribute__ ((noinline)) comp_test_write_good() { >>>>>>> + std::string data; >>>>>>> + >>>>>>> + char b; >>>>>>> + for (int i = 0; i < 2; ++i) { >>>>>>> + b = 1 >> (i * 8); >>>>>>> + data.append(&b, 1); >>>>>>> + } >>>>>>> + >>>>>>> + return data; >>>>>>> +} >>>>>>> + >>>>>>> +int main() { >>>>>>> + std::string good = comp_test_write_good(); >>>>>>> + printf("expected: %hx\n", *(short*)good.c_str()); >>>>>>> + >>>>>>> + std::string bad = comp_test_write(); >>>>>>> + printf("got: %hx\n", *(short*)bad.c_str()); >>>>>>> + >>>>>>> + return good != bad; >>>>>>> +} >>>>>>> >>>>>>> On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li <davidxl@google.com> wrote: >>>>>>>> missing test case? >>>>>>>> >>>>>>>> David >>>>>>>> >>>>>>>> On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>>>>>>> This patch fixes an issue where tree-strlen was incorrectly removing a >>>>>>>>> store of 0 into a string because it thought a prior CLOBBER (which is >>>>>>>>> an empty constructor with no elements) was zero-initializing the >>>>>>>>> string. >>>>>>>>> >>>>>>>>> Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Teresa >>>>>>>>> >>>>>>>>> 2014-11-12 <tejohnson@google.com> >>>>>>>>> >>>>>>>>> PR tree-optimization/63841 >>>>>>>>> * tree.c (initializer_zerop): A constructor with no elements >>>>>>>>> does not zero initialize. >>>>>>>>> >>>>>>>>> Index: tree.c >>>>>>>>> =================================================================== >>>>>>>>> --- tree.c (revision 217190) >>>>>>>>> +++ tree.c (working copy) >>>>>>>>> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >>>>>>>>> { >>>>>>>>> unsigned HOST_WIDE_INT idx; >>>>>>>>> >>>>>>>>> + if (!CONSTRUCTOR_NELTS (init)) >>>>>>>>> + return false; >>>>>>>>> FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) >>>>>>>>> if (!initializer_zerop (elt)) >>>>>>>>> return false; >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >>>>> >>>>> >>>>> >>>>> -- >>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >>> >>> >>> >>> -- >>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
On Thu, Nov 13, 2014 at 06:20:16AM -0800, Teresa Johnson wrote: > Here is the new patch. Bootstrapped and tested on > x86_64-unknown-linux-gnu. OK for trunk? > > Thanks, > Teresa > > 2014-11-13 <tejohnson@google.com> > > gcc: > PR tree-optimization/63841 > * tree.c (initializer_zerop): A constructor with no elements > does not zero initialize. > > gcc/testsuite: > PR tree-optimization/63841 > * g++.dg/tree-ssa/pr63841.C: New test. > > Index: tree.c > =================================================================== > --- tree.c (revision 217190) > +++ tree.c (working copy) > @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) > { > unsigned HOST_WIDE_INT idx; > > + if (TREE_CLOBBER_P (init)) > + return false; Wrong formatting. Also, while this perhaps is useful, I'd say the right fix is that strlen_optimize_stmt should just ignore gimple_clobber_p (stmt) statements (well, call the maybe_invalidate at the end for them). So - else if (is_gimple_assign (stmt)) + else if (is_gimple_assign (stmt) && !gimple_clobber_p (stmt)) in strlen_optimize_stmt. Jakub
On Thu, Nov 13, 2014 at 6:36 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Nov 13, 2014 at 06:20:16AM -0800, Teresa Johnson wrote: >> Here is the new patch. Bootstrapped and tested on >> x86_64-unknown-linux-gnu. OK for trunk? >> >> Thanks, >> Teresa >> >> 2014-11-13 <tejohnson@google.com> >> >> gcc: >> PR tree-optimization/63841 >> * tree.c (initializer_zerop): A constructor with no elements >> does not zero initialize. >> >> gcc/testsuite: >> PR tree-optimization/63841 >> * g++.dg/tree-ssa/pr63841.C: New test. >> >> Index: tree.c >> =================================================================== >> --- tree.c (revision 217190) >> +++ tree.c (working copy) >> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >> { >> unsigned HOST_WIDE_INT idx; >> >> + if (TREE_CLOBBER_P (init)) >> + return false; > > Wrong formatting. Sorry, not sure I understand why? My mailer does tend to eat spaces, but it is indented the correct amount and I think has the right spaces within the line. > > Also, while this perhaps is useful, I'd say the right fix is that strlen_optimize_stmt > should just ignore gimple_clobber_p (stmt) statements (well, call > the maybe_invalidate at the end for them). > So > - else if (is_gimple_assign (stmt)) > + else if (is_gimple_assign (stmt) && !gimple_clobber_p (stmt)) > in strlen_optimize_stmt. Ok, I have held off on my commit for now. I considered fixing this in tree-ssa-strlen, but thought this was a potentially larger problem with initializer_zerop, where it shouldn't be considering a clobber to be a zero init and might be affecting other callers as well. If we make the change to initializer_zerop is it still useful to change tree-strlen? Teresa > > Jakub
On Thu, Nov 13, 2014 at 3:46 PM, Teresa Johnson <tejohnson@google.com> wrote: > On Thu, Nov 13, 2014 at 6:36 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Thu, Nov 13, 2014 at 06:20:16AM -0800, Teresa Johnson wrote: >>> Here is the new patch. Bootstrapped and tested on >>> x86_64-unknown-linux-gnu. OK for trunk? >>> >>> Thanks, >>> Teresa >>> >>> 2014-11-13 <tejohnson@google.com> >>> >>> gcc: >>> PR tree-optimization/63841 >>> * tree.c (initializer_zerop): A constructor with no elements >>> does not zero initialize. >>> >>> gcc/testsuite: >>> PR tree-optimization/63841 >>> * g++.dg/tree-ssa/pr63841.C: New test. >>> >>> Index: tree.c >>> =================================================================== >>> --- tree.c (revision 217190) >>> +++ tree.c (working copy) >>> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >>> { >>> unsigned HOST_WIDE_INT idx; >>> >>> + if (TREE_CLOBBER_P (init)) >>> + return false; >> >> Wrong formatting. > > Sorry, not sure I understand why? My mailer does tend to eat spaces, > but it is indented the correct amount and I think has the right spaces > within the line. > >> >> Also, while this perhaps is useful, I'd say the right fix is that strlen_optimize_stmt >> should just ignore gimple_clobber_p (stmt) statements (well, call >> the maybe_invalidate at the end for them). >> So >> - else if (is_gimple_assign (stmt)) >> + else if (is_gimple_assign (stmt) && !gimple_clobber_p (stmt)) >> in strlen_optimize_stmt. > > Ok, I have held off on my commit for now. I considered fixing this in > tree-ssa-strlen, but thought this was a potentially larger problem > with initializer_zerop, where it shouldn't be considering a clobber to > be a zero init and might be affecting other callers as well. I think it is. Richard. > If we make the change to initializer_zerop is it still useful to > change tree-strlen? > > Teresa > >> >> Jakub > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
On Thu, Nov 13, 2014 at 06:46:23AM -0800, Teresa Johnson wrote: > >> --- tree.c (revision 217190) > >> +++ tree.c (working copy) > >> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) > >> { > >> unsigned HOST_WIDE_INT idx; > >> > >> + if (TREE_CLOBBER_P (init)) > >> + return false; > > > > Wrong formatting. > > Sorry, not sure I understand why? My mailer does tend to eat spaces, > but it is indented the correct amount and I think has the right spaces > within the line. I meant that you are using spaces instead of tabs and the unsigned line above it being one char off suggested to me visually it has the tab in there (apparently it is eaten too). Use MUA that doesn't eat it, or convince your employer that tabs are important in e-mails? ;) If you commit tabs in there, it is fine with me. > Ok, I have held off on my commit for now. I considered fixing this in > tree-ssa-strlen, but thought this was a potentially larger problem > with initializer_zerop, where it shouldn't be considering a clobber to > be a zero init and might be affecting other callers as well. As I said, I think your tree.c change is useful, but perhaps too risky for the release branches, because we don't know what all it will affect? > If we make the change to initializer_zerop is it still useful to > change tree-strlen? I think it is better to be clear on it that right now we want clobbers to clobber the memory for strlen pass purposes, maybe in the future we want to perform some optimizations for them as Richard suggested in the PR (though, extending the lifetime in that case by removing the clobber shouldn't be done unconditionally (i.e. limited to when we'd actually want to benefit from the known length) and assuming we are close to the clobber (i.e. it is fine to extend it a little bit, but not extend the lifetime across multi-megabyte function e.g.). And for release branches I'd really prefer tree-ssa-strlen.c change. Jakub
On Thu, Nov 13, 2014 at 7:12 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Nov 13, 2014 at 06:46:23AM -0800, Teresa Johnson wrote: >> >> --- tree.c (revision 217190) >> >> +++ tree.c (working copy) >> >> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >> >> { >> >> unsigned HOST_WIDE_INT idx; >> >> >> >> + if (TREE_CLOBBER_P (init)) >> >> + return false; >> > >> > Wrong formatting. >> >> Sorry, not sure I understand why? My mailer does tend to eat spaces, >> but it is indented the correct amount and I think has the right spaces >> within the line. > > I meant that you are using spaces instead of tabs and the unsigned line > above it being one char off suggested to me visually it has the tab in there > (apparently it is eaten too). Use MUA that doesn't eat it, or convince your > employer that tabs are important in e-mails? ;) > > If you commit tabs in there, it is fine with me. Changed the spaces to tabs to match the surrounding lines and committed to trunk (r217505). > >> Ok, I have held off on my commit for now. I considered fixing this in >> tree-ssa-strlen, but thought this was a potentially larger problem >> with initializer_zerop, where it shouldn't be considering a clobber to >> be a zero init and might be affecting other callers as well. > > As I said, I think your tree.c change is useful, but perhaps too risky > for the release branches, because we don't know what all it will affect? > >> If we make the change to initializer_zerop is it still useful to >> change tree-strlen? > > I think it is better to be clear on it that right now we want clobbers > to clobber the memory for strlen pass purposes, maybe in the future we want > to perform some optimizations for them as Richard suggested in the PR > (though, extending the lifetime in that case by removing the clobber > shouldn't be done unconditionally (i.e. limited to when we'd actually want > to benefit from the known length) and assuming we are close to the clobber > (i.e. it is fine to extend it a little bit, but not extend the lifetime > across multi-megabyte function e.g.). > And for release branches I'd really prefer tree-ssa-strlen.c change. Ok, I started testing the initializer_zerop change on the 4_9 branch, will also test the strlen fix and send that patch for review here when it completes. Thanks, Teresa > > Jakub
Index: tree.c =================================================================== --- tree.c (revision 217190) +++ tree.c (working copy) @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) { unsigned HOST_WIDE_INT idx; + if (TREE_CLOBBER_P (init)) + return false; FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) if (!initializer_zerop (elt)) return false; Index: testsuite/g++.dg/tree-ssa/pr63841.C =================================================================== --- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0) +++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy) @@ -0,0 +1,38 @@ +/* { dg-do run } */ +/* { dg-options "-O2" } */ + +#include <cstdio> +#include <string> + +std::string __attribute__ ((noinline)) comp_test_write() { + std::string data; + + for (int i = 0; i < 2; ++i) { + char b = 1 >> (i * 8); + data.append(&b, 1); + } + + return data; +} + +std::string __attribute__ ((noinline)) comp_test_write_good() { + std::string data; + + char b; + for (int i = 0; i < 2; ++i) { + b = 1 >> (i * 8); + data.append(&b, 1); + } + + return data; +} + +int main() { + std::string good = comp_test_write_good(); + printf("expected: %hx\n", *(short*)good.c_str()); + + std::string bad = comp_test_write(); + printf("got: %hx\n", *(short*)bad.c_str()); + + return good != bad; +}
Here is the new patch. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? Thanks, Teresa 2014-11-13 <tejohnson@google.com> gcc: PR tree-optimization/63841 * tree.c (initializer_zerop): A constructor with no elements does not zero initialize. gcc/testsuite: PR tree-optimization/63841 * g++.dg/tree-ssa/pr63841.C: New test. On Wed, Nov 12, 2014 at 9:40 PM, Andrew Pinski <pinskia@gmail.com> wrote: > On Wed, Nov 12, 2014 at 9:38 PM, Teresa Johnson <tejohnson@google.com> wrote: >> On Wed, Nov 12, 2014 at 9:30 PM, Andrew Pinski <pinskia@gmail.com> wrote: >>> On Wed, Nov 12, 2014 at 9:25 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>> Added testcase. Here is the new patch: >>>> >>>> 2014-11-12 <tejohnson@google.com> >>>> >>>> gcc: >>>> PR tree-optimization/63841 >>>> * tree.c (initializer_zerop): A constructor with no elements >>>> does not zero initialize. >>> >>> Actually an empty constructor does zero initialize. A clobber does not. >> >> Ok, thanks, I wasn't sure. >> >>> >>> The check you want is TREE_CLOBBER_P instead. >> >> So in initializer_zerop, a constructor that is a TREE_CLOBBER_P would >> return false, right? I will make that change and retest. > > Yes that is correct. Note TREE_CLOBBER_P already checks if it is an > constructor. > > Thanks, > Andrew > >> >> Thanks, >> Teresa >> >>> >>> Thanks, >>> Andrew >>> >>> >>>> >>>> gcc/testsuite: >>>> * g++.dg/tree-ssa/pr63841.C: New test. >>>> >>>> Index: tree.c >>>> =================================================================== >>>> --- tree.c (revision 217190) >>>> +++ tree.c (working copy) >>>> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >>>> { >>>> unsigned HOST_WIDE_INT idx; >>>> >>>> + if (!CONSTRUCTOR_NELTS (init)) >>>> + return false; >>>> FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) >>>> if (!initializer_zerop (elt)) >>>> return false; >>>> Index: testsuite/g++.dg/tree-ssa/pr63841.C >>>> =================================================================== >>>> --- testsuite/g++.dg/tree-ssa/pr63841.C (revision 0) >>>> +++ testsuite/g++.dg/tree-ssa/pr63841.C (working copy) >>>> @@ -0,0 +1,38 @@ >>>> +/* { dg-do run } */ >>>> +/* { dg-options "-O2" } */ >>>> + >>>> +#include <cstdio> >>>> +#include <string> >>>> + >>>> +std::string __attribute__ ((noinline)) comp_test_write() { >>>> + std::string data; >>>> + >>>> + for (int i = 0; i < 2; ++i) { >>>> + char b = 1 >> (i * 8); >>>> + data.append(&b, 1); >>>> + } >>>> + >>>> + return data; >>>> +} >>>> + >>>> +std::string __attribute__ ((noinline)) comp_test_write_good() { >>>> + std::string data; >>>> + >>>> + char b; >>>> + for (int i = 0; i < 2; ++i) { >>>> + b = 1 >> (i * 8); >>>> + data.append(&b, 1); >>>> + } >>>> + >>>> + return data; >>>> +} >>>> + >>>> +int main() { >>>> + std::string good = comp_test_write_good(); >>>> + printf("expected: %hx\n", *(short*)good.c_str()); >>>> + >>>> + std::string bad = comp_test_write(); >>>> + printf("got: %hx\n", *(short*)bad.c_str()); >>>> + >>>> + return good != bad; >>>> +} >>>> >>>> On Wed, Nov 12, 2014 at 2:17 PM, Xinliang David Li <davidxl@google.com> wrote: >>>>> missing test case? >>>>> >>>>> David >>>>> >>>>> On Wed, Nov 12, 2014 at 2:13 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>>>> This patch fixes an issue where tree-strlen was incorrectly removing a >>>>>> store of 0 into a string because it thought a prior CLOBBER (which is >>>>>> an empty constructor with no elements) was zero-initializing the >>>>>> string. >>>>>> >>>>>> Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? >>>>>> >>>>>> Thanks, >>>>>> Teresa >>>>>> >>>>>> 2014-11-12 <tejohnson@google.com> >>>>>> >>>>>> PR tree-optimization/63841 >>>>>> * tree.c (initializer_zerop): A constructor with no elements >>>>>> does not zero initialize. >>>>>> >>>>>> Index: tree.c >>>>>> =================================================================== >>>>>> --- tree.c (revision 217190) >>>>>> +++ tree.c (working copy) >>>>>> @@ -10330,6 +10330,8 @@ initializer_zerop (const_tree init) >>>>>> { >>>>>> unsigned HOST_WIDE_INT idx; >>>>>> >>>>>> + if (!CONSTRUCTOR_NELTS (init)) >>>>>> + return false; >>>>>> FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (init), idx, elt) >>>>>> if (!initializer_zerop (elt)) >>>>>> return false; >>>>>> >>>>>> >>>>>> -- >>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >>>> >>>> >>>> >>>> -- >>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >> >> >> >> -- >> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413