Re: [PATCH] Fix for PR64353

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

On 16 Jan 12:38, Richard Biener wrote:
> On Fri, Jan 16, 2015 at 11:51 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> > On 14 Jan 19:40, Richard Biener wrote:
> >> On January 14, 2015 5:23:21 PM CET, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> >> >On 14 Jan 15:35, Richard Biener wrote:
> >> >> On Wed, Jan 14, 2015 at 3:28 PM, Ilya Enkovich
> >> ><enkovich.gnu@gmail.com> wrote:
> >> >> > Hi,
> >> >> >
> >> >> > SRA gimple passes may add loads to functions with no SSA update.
> >> >Later it causes ICE when function with not updated SSA is processed by
> >> >gimple passes.  This patch fixes it by calling update_ssa.
> >> >> >
> >> >> > Bootstrapped and checked on x86_64-unknown-linux-gnu.  OK for
> >> >trunk?
> >> >>
> >> >> No.  I have removed this quadratic update-ssa call previously.  It
> >> >should
> >> >> simply keep SSA for up-to-date manually (see how it does
> >> >gimple_set_vuse
> >> >> in some cases, probably not all required ones?).
> >> >>
> >> >
> >> >Would it be OK to call update_ssa only in case we don't have a proper
> >> >VUSE for call?
> >>
> >> No, and most definitely not here.
> >>
> >>
> >> Are we allowed to just emit error due to incorrect
> >> >attribute?
> >>
> >> No, I don't think so either. But we may drop it.
> >>
> >> Richard.
> >>
> >
> > Here is a version with SRA disabled for functions with __attribute__((const)) as you suggested in tracker.  Is it OK?
> >
> > Bootstrapped and checked on x86_64-unknown-linux-gnu.
> >
> > Thanks,
> > Ilya
> > --
> > gcc/
> >
> > 2015-01-16  Ilya Enkovich  <ilya.enkovich@intel.com>
> >
> >         PR middle-end/64353
> >         * tree-sra.c (ipa_sra_preliminary_function_checks): Reject
> >         functions with const attribute.
> >
> > gcc/testsuite/
> >
> > 2015-01-16  Ilya Enkovich  <ilya.enkovich@intel.com>
> >
> >         PR middle-end/64353
> >         * g++.dg/pr64353.C: New.
> >
> >
> > diff --git a/gcc/testsuite/g++.dg/pr64353.C b/gcc/testsuite/g++.dg/pr64353.C
> > new file mode 100644
> > index 0000000..7859918
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/pr64353.C
> > @@ -0,0 +1,15 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2" } */
> > +
> > +class C
> > +{
> > +  int y, x;
> > +  void i ();
> > +  bool __attribute__((const)) xx () { return x; }
> > +};
> > +
> > +void C::i ()
> > +{
> > +  if (xx ())
> > +    x = 1;
> > +}
> > diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> > index f560fe0..f24ca9f 100644
> > --- a/gcc/tree-sra.c
> > +++ b/gcc/tree-sra.c
> > @@ -5015,6 +5015,13 @@ ipa_sra_preliminary_function_checks (struct cgraph_node *node)
> >        return false;
> >      }
> >
> > +  if (lookup_attribute ("const", DECL_ATTRIBUTES (node->decl)))
> 
> TREE_READONLY (node->decl)
> 
> but I'm now not sure that we can really trust this given that only fixup_cfg
> will eventually fixup stmts in callers after ipa-pure-const ran.  The above
> also doesn't handle "no vops" functions.
> 
> I think a better place to check this would be inside
> some_callers_have_mismatched_arguments_p where you'd check
> 
>   || !gimple_vuse (cs->call_stmt)
> 
> of course the reasoning printed is wrong then.
> 
> We can fix this by running update-ssa in fixup_cfg as well, and I guess
> I like that more.
> 
> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c  (revision 219714)
> +++ tree-cfg.c  (working copy)
> @@ -8754,7 +8804,7 @@ const pass_data pass_data_fixup_cfg =
>    PROP_cfg, /* properties_required */
>    0, /* properties_provided */
>    0, /* properties_destroyed */
> -  0, /* todo_flags_start */
> +  TODO_update_ssa_only_virtuals, /* todo_flags_start */
>    0, /* todo_flags_finish */
>  };
> 
> this variant is pre-approved if it passes bootstrap and regtest.
> 
> Thanks,
> Richard.
> 

Bootstrap and regtest is OK on x86_64-unknown-linux-gnu.  Here is a committed patch.

Thanks,
Ilya
--
gcc/

2015-01-16  Ilya Enkovich  <ilya.enkovich@intel.com>

	PR middle-end/64353
	* tree-cfg.c (pass_data_fixup_cfg): Update SSA for
	virtuals on start.

gcc/testsuite/

2015-01-16  Ilya Enkovich  <ilya.enkovich@intel.com>

	PR middle-end/64353
	* g++.dg/pr64353.C: New.


diff --git a/gcc/testsuite/g++.dg/pr64353.C b/gcc/testsuite/g++.dg/pr64353.C
new file mode 100644
index 0000000..7859918
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr64353.C
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+class C
+{
+  int y, x;
+  void i ();
+  bool __attribute__((const)) xx () { return x; }
+};
+
+void C::i ()
+{
+  if (xx ())
+    x = 1;
+}
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index a9a2c2f..b56a453 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -8741,7 +8741,7 @@ const pass_data pass_data_fixup_cfg =
   PROP_cfg, /* properties_required */
   0, /* properties_provided */
   0, /* properties_destroyed */
-  0, /* todo_flags_start */
+  TODO_update_ssa_only_virtuals, /* todo_flags_start */
   0, /* todo_flags_finish */
 };