Fix for PR64353
diff mbox

Message ID CAFiYyc2bu5BUb0zJCUNjXLHavzC5hS3F87iwAcsp73te6v26TQ@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener Jan. 16, 2015, 11:38 a.m. UTC
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.

this variant is pre-approved if it passes bootstrap and regtest.

Thanks,
Richard.

> +    {
> +      if (dump_file)
> +       fprintf (dump_file, "Function has const attribute.\n");
> +      return false;
> +    }
> +
>    if (!tree_versionable_function_p (node->decl))
>      {
>        if (dump_file)

Patch
diff mbox

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 */
 };