| Message ID | 20150212141419.GA35812@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 12, 2015 at 3:14 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > Hi, > > PR65002 is the second issue caused by SRA for functions wrongly marked as read-only. Previous fix for PR64353 doesn't work for this case because SSA update happens too late. Here is a patch to disable SRA for such functions to avoid inconsistent SSA issues. > > Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? Ok. I'm not entirely happy with this but I guess any other solution has to wait for GCC 6. Thanks, Richard. > Thanks, > Ilya > -- > gcc/ > > 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> > > PR tree-optimization/65002 > * tree-cfg.c (pass_data_fixup_cfg): Don't update > SSA on start. > * tree-sra.c (some_callers_have_no_vuse_p): New. > (ipa_early_sra): Reject functions whose callers > assume funciton is read only. > > gcc/testsuite/ > > 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> > > PR tree-optimization/65002 > * gcc.dg/pr65002.C: New. > > > diff --git a/gcc/testsuite/gcc.dg/pr65002.C b/gcc/testsuite/gcc.dg/pr65002.C > new file mode 100644 > index 0000000..ac7c66b > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr65002.C > @@ -0,0 +1,26 @@ > +/* PR tree-optimization/65002 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +namespace fastmath { > + template <typename T> float floor(const T &) __attribute__((const)); > + template <typename T> float floor(const T &p1) { return p1; } > +} > +using fastmath::floor; > +class A { > +public: > + A(int, int); > + virtual int m_fn1(float) const; > +}; > +class B : A { > +public: > + B(int, int p2) : A(entity, p2) {} > + int m_fn1(float p1) const { long b(floor(p1)); } > + int entity; > +}; > + > +int a; > +void Convert() { > + if (int *c = 0) > + B(*c, a); > +} > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > index 2e23553..006bc08 100644 > --- a/gcc/tree-cfg.c > +++ b/gcc/tree-cfg.c > @@ -8754,7 +8754,7 @@ const pass_data pass_data_fixup_cfg = > PROP_cfg, /* properties_required */ > 0, /* properties_provided */ > 0, /* properties_destroyed */ > - TODO_update_ssa_only_virtuals, /* todo_flags_start */ > + 0, /* todo_flags_start */ > 0, /* todo_flags_finish */ > }; > > diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c > index ad9584e..7f78e68 100644 > --- a/gcc/tree-sra.c > +++ b/gcc/tree-sra.c > @@ -4890,6 +4890,20 @@ some_callers_have_mismatched_arguments_p (struct cgraph_node *node, > return false; > } > > +/* Return false if all callers have vuse attached to a call statement. */ > + > +static bool > +some_callers_have_no_vuse_p (struct cgraph_node *node, > + void *data ATTRIBUTE_UNUSED) > +{ > + struct cgraph_edge *cs; > + for (cs = node->callers; cs; cs = cs->next_caller) > + if (!cs->call_stmt || !gimple_vuse (cs->call_stmt)) > + return true; > + > + return false; > +} > + > /* Convert all callers of NODE. */ > > static bool > @@ -5116,6 +5130,15 @@ ipa_early_sra (void) > goto simple_out; > } > > + if (node->call_for_symbol_thunks_and_aliases > + (some_callers_have_no_vuse_p, NULL, true)) > + { > + if (dump_file) > + fprintf (dump_file, "There are callers with no VUSE attached " > + "to a call stmt.\n"); > + goto simple_out; > + } > + > bb_dereferences = XCNEWVEC (HOST_WIDE_INT, > func_param_count > * last_basic_block_for_fn (cfun));
On Fri, Feb 13, 2015 at 09:47:56AM +0100, Richard Biener wrote: > > 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> > > > > PR tree-optimization/65002 > > * tree-cfg.c (pass_data_fixup_cfg): Don't update > > SSA on start. > > * tree-sra.c (some_callers_have_no_vuse_p): New. > > (ipa_early_sra): Reject functions whose callers > > assume funciton is read only. Typo, function. > > +static bool > > +some_callers_have_no_vuse_p (struct cgraph_node *node, > > + void *data ATTRIBUTE_UNUSED) > > +{ > > + struct cgraph_edge *cs; > > + for (cs = node->callers; cs; cs = cs->next_caller) > > + if (!cs->call_stmt || !gimple_vuse (cs->call_stmt)) > > + return true; > > + > > + return false; > > +} > > + > > /* Convert all callers of NODE. */ > > > > static bool > > @@ -5116,6 +5130,15 @@ ipa_early_sra (void) > > goto simple_out; > > } > > > > + if (node->call_for_symbol_thunks_and_aliases > > + (some_callers_have_no_vuse_p, NULL, true)) > > + { > > + if (dump_file) > > + fprintf (dump_file, "There are callers with no VUSE attached " > > + "to a call stmt.\n"); > > + goto simple_out; > > + } > > + I wonder if this won't pessimize const functions that just get called with aggregate arguments passed by value, do those count as memory read or just as parameters? Jakub
2015-02-13 11:54 GMT+03:00 Jakub Jelinek <jakub@redhat.com>: > On Fri, Feb 13, 2015 at 09:47:56AM +0100, Richard Biener wrote: >> > 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> >> > >> > PR tree-optimization/65002 >> > * tree-cfg.c (pass_data_fixup_cfg): Don't update >> > SSA on start. >> > * tree-sra.c (some_callers_have_no_vuse_p): New. >> > (ipa_early_sra): Reject functions whose callers >> > assume funciton is read only. > > Typo, function. > >> > +static bool >> > +some_callers_have_no_vuse_p (struct cgraph_node *node, >> > + void *data ATTRIBUTE_UNUSED) >> > +{ >> > + struct cgraph_edge *cs; >> > + for (cs = node->callers; cs; cs = cs->next_caller) >> > + if (!cs->call_stmt || !gimple_vuse (cs->call_stmt)) >> > + return true; >> > + >> > + return false; >> > +} >> > + >> > /* Convert all callers of NODE. */ >> > >> > static bool >> > @@ -5116,6 +5130,15 @@ ipa_early_sra (void) >> > goto simple_out; >> > } >> > >> > + if (node->call_for_symbol_thunks_and_aliases >> > + (some_callers_have_no_vuse_p, NULL, true)) >> > + { >> > + if (dump_file) >> > + fprintf (dump_file, "There are callers with no VUSE attached " >> > + "to a call stmt.\n"); >> > + goto simple_out; >> > + } >> > + > > I wonder if this won't pessimize const functions that just get called with > aggregate arguments passed by value, do those count as memory read or > just as parameters? Tried it on a simple test. struct S { int a; int b; }; int f1 (struct S s) __attribute__((const)); int f2 (struct S *ps) { return f1 (*ps); } There is a memory use for a call: <bb 2>: # VUSE <.MEM_1(D)> _3 = f1 (*ps_2(D)); But if wrongly marked const function has all calls with vuses for some reason then we just should be able to optimize it with a proper SSA update. Ilya > > Jakub
On Fri, Feb 13, 2015 at 9:54 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Feb 13, 2015 at 09:47:56AM +0100, Richard Biener wrote: >> > 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> >> > >> > PR tree-optimization/65002 >> > * tree-cfg.c (pass_data_fixup_cfg): Don't update >> > SSA on start. >> > * tree-sra.c (some_callers_have_no_vuse_p): New. >> > (ipa_early_sra): Reject functions whose callers >> > assume funciton is read only. > > Typo, function. > >> > +static bool >> > +some_callers_have_no_vuse_p (struct cgraph_node *node, >> > + void *data ATTRIBUTE_UNUSED) >> > +{ >> > + struct cgraph_edge *cs; >> > + for (cs = node->callers; cs; cs = cs->next_caller) >> > + if (!cs->call_stmt || !gimple_vuse (cs->call_stmt)) >> > + return true; >> > + >> > + return false; >> > +} >> > + >> > /* Convert all callers of NODE. */ >> > >> > static bool >> > @@ -5116,6 +5130,15 @@ ipa_early_sra (void) >> > goto simple_out; >> > } >> > >> > + if (node->call_for_symbol_thunks_and_aliases >> > + (some_callers_have_no_vuse_p, NULL, true)) >> > + { >> > + if (dump_file) >> > + fprintf (dump_file, "There are callers with no VUSE attached " >> > + "to a call stmt.\n"); >> > + goto simple_out; >> > + } >> > + > > I wonder if this won't pessimize const functions that just get called with > aggregate arguments passed by value, do those count as memory read or > just as parameters? They count as memory reads and thus cause a VUSE (similar to aggregate returns causing a VDEF). Richard. > Jakub
On Fri, Feb 13, 2015 at 10:21:42AM +0100, Richard Biener wrote: > > I wonder if this won't pessimize const functions that just get called with > > aggregate arguments passed by value, do those count as memory read or > > just as parameters? > > They count as memory reads and thus cause a VUSE (similar to aggregate > returns causing a VDEF). Ok. Jakub
On Thu, Feb 12, 2015 at 05:14:19PM +0300, Ilya Enkovich wrote: > Hi, > > PR65002 is the second issue caused by SRA for functions wrongly marked as read-only. Previous fix for PR64353 doesn't work for this case because SSA update happens too late. Here is a patch to disable SRA for such functions to avoid inconsistent SSA issues. > > Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? > > Thanks, > Ilya > -- > gcc/ > > 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> > > PR tree-optimization/65002 > * tree-cfg.c (pass_data_fixup_cfg): Don't update > SSA on start. > * tree-sra.c (some_callers_have_no_vuse_p): New. > (ipa_early_sra): Reject functions whose callers > assume funciton is read only. > > gcc/testsuite/ > > 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> > > PR tree-optimization/65002 > * gcc.dg/pr65002.C: New. This test should have gone into g++.dg. Marek
On Fri, Feb 13, 2015 at 08:19:52PM +0100, Marek Polacek wrote: > On Thu, Feb 12, 2015 at 05:14:19PM +0300, Ilya Enkovich wrote: > > Hi, > > > > PR65002 is the second issue caused by SRA for functions wrongly marked as read-only. Previous fix for PR64353 doesn't work for this case because SSA update happens too late. Here is a patch to disable SRA for such functions to avoid inconsistent SSA issues. > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? > > > > Thanks, > > Ilya > > -- > > gcc/ > > > > 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> > > > > PR tree-optimization/65002 > > * tree-cfg.c (pass_data_fixup_cfg): Don't update > > SSA on start. > > * tree-sra.c (some_callers_have_no_vuse_p): New. > > (ipa_early_sra): Reject functions whose callers > > assume funciton is read only. > > > > gcc/testsuite/ > > > > 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> > > > > PR tree-optimization/65002 > > * gcc.dg/pr65002.C: New. > > This test should have gone into g++.dg. Into g++.dg/opt or g++.dg/ipa in particular. Jakub
On Feb 13, 2015, at 11:25 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>> 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> PR tree-optimization/65002 >>> * gcc.dg/pr65002.C: New. >> >> This test should have gone into g++.dg. > > Into g++.dg/opt or g++.dg/ipa in particular. Pre-approved if someone wants to svn mv it.
2015-02-15 20:25 GMT+03:00 Mike Stump <mikestump@comcast.net>: > On Feb 13, 2015, at 11:25 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>> 2015-02-12 Ilya Enkovich <ilya.enkovich@intel.com> >>>> >>>> PR tree-optimization/65002 >>>> * gcc.dg/pr65002.C: New. >>> >>> This test should have gone into g++.dg. >> >> Into g++.dg/opt or g++.dg/ipa in particular. > > Pre-approved if someone wants to svn mv it. Moved it. 2015-02-16 Ilya Enkovich <ilya.enkovich@intel.com> * gcc.dg/pr65002.C: Move ... * g++.dg/ipa/pr65002.C: ... here. Thanks, Ilya
diff --git a/gcc/testsuite/gcc.dg/pr65002.C b/gcc/testsuite/gcc.dg/pr65002.C new file mode 100644 index 0000000..ac7c66b --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr65002.C @@ -0,0 +1,26 @@ +/* PR tree-optimization/65002 */ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +namespace fastmath { + template <typename T> float floor(const T &) __attribute__((const)); + template <typename T> float floor(const T &p1) { return p1; } +} +using fastmath::floor; +class A { +public: + A(int, int); + virtual int m_fn1(float) const; +}; +class B : A { +public: + B(int, int p2) : A(entity, p2) {} + int m_fn1(float p1) const { long b(floor(p1)); } + int entity; +}; + +int a; +void Convert() { + if (int *c = 0) + B(*c, a); +} diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index 2e23553..006bc08 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -8754,7 +8754,7 @@ const pass_data pass_data_fixup_cfg = PROP_cfg, /* properties_required */ 0, /* properties_provided */ 0, /* properties_destroyed */ - TODO_update_ssa_only_virtuals, /* todo_flags_start */ + 0, /* todo_flags_start */ 0, /* todo_flags_finish */ }; diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c index ad9584e..7f78e68 100644 --- a/gcc/tree-sra.c +++ b/gcc/tree-sra.c @@ -4890,6 +4890,20 @@ some_callers_have_mismatched_arguments_p (struct cgraph_node *node, return false; } +/* Return false if all callers have vuse attached to a call statement. */ + +static bool +some_callers_have_no_vuse_p (struct cgraph_node *node, + void *data ATTRIBUTE_UNUSED) +{ + struct cgraph_edge *cs; + for (cs = node->callers; cs; cs = cs->next_caller) + if (!cs->call_stmt || !gimple_vuse (cs->call_stmt)) + return true; + + return false; +} + /* Convert all callers of NODE. */ static bool @@ -5116,6 +5130,15 @@ ipa_early_sra (void) goto simple_out; } + if (node->call_for_symbol_thunks_and_aliases + (some_callers_have_no_vuse_p, NULL, true)) + { + if (dump_file) + fprintf (dump_file, "There are callers with no VUSE attached " + "to a call stmt.\n"); + goto simple_out; + } + bb_dereferences = XCNEWVEC (HOST_WIDE_INT, func_param_count * last_basic_block_for_fn (cfun));