[1/2] Do not verify CFG if a function is discarded (PR
diff mbox

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

Commit Message

Richard Biener March 29, 2016, 12:10 p.m. UTC
On Tue, Mar 29, 2016 at 1:42 PM, Martin Liška <mliska@suse.cz> wrote:
> Hello.
>
> The problem with the original patch is that I'm forced to produce
> an empty BB to produce true/false edge needed for the 'index' check:
>
> /home/marxin/Programming/testhsa/run_tests/012-switch/switch-5.c:28:9: error: true/false edge after a non-GIMPLE_COND in bb 4
> /home/marxin/Programming/testhsa/run_tests/012-switch/switch-5.c:28:9: internal compiler error: verify_flow_info failed
> 0x93121a verify_flow_info()
>         ../../gcc/cfghooks.c:260
> 0xd5ae4e execute_function_todo
>         ../../gcc/passes.c:1971
> 0xd59ea6 do_per_function
>         ../../gcc/passes.c:1645
> 0xd5afc2 execute_todo
>         ../../gcc/passes.c:2011
>
> It would nicer to not produce empty block for that purpose, but the question
> is if the change is acceptable during the stage4?

Hmm, why don't we short-cut things earlier in execute_one_pass where
we handle TODO_discard_function?
That is, sth like



> Thanks,
> Martin

Comments

Martin Liška March 29, 2016, 1:07 p.m. UTC | #1
On 03/29/2016 02:10 PM, Richard Biener wrote:
> On Tue, Mar 29, 2016 at 1:42 PM, Martin Liška <mliska@suse.cz> wrote:
>> Hello.
>>
>> The problem with the original patch is that I'm forced to produce
>> an empty BB to produce true/false edge needed for the 'index' check:
>>
>> /home/marxin/Programming/testhsa/run_tests/012-switch/switch-5.c:28:9: error: true/false edge after a non-GIMPLE_COND in bb 4
>> /home/marxin/Programming/testhsa/run_tests/012-switch/switch-5.c:28:9: internal compiler error: verify_flow_info failed
>> 0x93121a verify_flow_info()
>>         ../../gcc/cfghooks.c:260
>> 0xd5ae4e execute_function_todo
>>         ../../gcc/passes.c:1971
>> 0xd59ea6 do_per_function
>>         ../../gcc/passes.c:1645
>> 0xd5afc2 execute_todo
>>         ../../gcc/passes.c:2011
>>
>> It would nicer to not produce empty block for that purpose, but the question
>> is if the change is acceptable during the stage4?
> 
> Hmm, why don't we short-cut things earlier in execute_one_pass where
> we handle TODO_discard_function?
> That is, sth like
> 
> Index: gcc/passes.c
> ===================================================================
> --- gcc/passes.c        (revision 234453)
> +++ gcc/passes.c        (working copy)
> @@ -2334,6 +2334,33 @@ execute_one_pass (opt_pass *pass)
> 
>    /* Do it!  */
>    todo_after = pass->execute (cfun);
> +
> +  if (todo_after & TODO_discard_function)
> +    {
> +      pass_fini_dump_file (pass);
> +
> +      gcc_assert (cfun);
> +      /* As cgraph_node::release_body expects release dominators info,
> +        we have to release it.  */
> +      if (dom_info_available_p (CDI_DOMINATORS))
> +       free_dominance_info (CDI_DOMINATORS);
> +
> +      if (dom_info_available_p (CDI_POST_DOMINATORS))
> +       free_dominance_info (CDI_POST_DOMINATORS);
> +
> +      tree fn = cfun->decl;
> +      pop_cfun ();
> +      gcc_assert (!cfun);
> +      cgraph_node::get (fn)->release_body ();
> +
> +      current_pass = NULL;
> +      redirect_edge_var_map_empty ();
> +
> +      ggc_collect ();
> +
> +      return true;
> +    }
> +
>    do_per_function (clear_last_verified, NULL);
> 
>    /* Stop timevar.  */
> @@ -2373,23 +2400,6 @@ execute_one_pass (opt_pass *pass)
>    current_pass = NULL;
>    redirect_edge_var_map_empty ();
> 
> -  if (todo_after & TODO_discard_function)
> -    {
> -      gcc_assert (cfun);
> -      /* As cgraph_node::release_body expects release dominators info,
> -        we have to release it.  */
> -      if (dom_info_available_p (CDI_DOMINATORS))
> -       free_dominance_info (CDI_DOMINATORS);
> -
> -      if (dom_info_available_p (CDI_POST_DOMINATORS))
> -       free_dominance_info (CDI_POST_DOMINATORS);
> -
> -      tree fn = cfun->decl;
> -      pop_cfun ();
> -      gcc_assert (!cfun);
> -      cgraph_node::get (fn)->release_body ();
> -    }
> -
>    /* Signal this is a suitable GC collection point.  */
>    if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
>      ggc_collect ();
> 
> 

Hello Richi.

Thanks for coming with the cleaner version of the patch.
I've incorporated that patch and reg&bootstrap is running.

Installable as soon as it finishes?

Thanks,
Martin

>> Thanks,
>> Martin
Richard Biener March 29, 2016, 2:08 p.m. UTC | #2
On Tue, Mar 29, 2016 at 3:07 PM, Martin Liška <mliska@suse.cz> wrote:
> On 03/29/2016 02:10 PM, Richard Biener wrote:
>> On Tue, Mar 29, 2016 at 1:42 PM, Martin Liška <mliska@suse.cz> wrote:
>>> Hello.
>>>
>>> The problem with the original patch is that I'm forced to produce
>>> an empty BB to produce true/false edge needed for the 'index' check:
>>>
>>> /home/marxin/Programming/testhsa/run_tests/012-switch/switch-5.c:28:9: error: true/false edge after a non-GIMPLE_COND in bb 4
>>> /home/marxin/Programming/testhsa/run_tests/012-switch/switch-5.c:28:9: internal compiler error: verify_flow_info failed
>>> 0x93121a verify_flow_info()
>>>         ../../gcc/cfghooks.c:260
>>> 0xd5ae4e execute_function_todo
>>>         ../../gcc/passes.c:1971
>>> 0xd59ea6 do_per_function
>>>         ../../gcc/passes.c:1645
>>> 0xd5afc2 execute_todo
>>>         ../../gcc/passes.c:2011
>>>
>>> It would nicer to not produce empty block for that purpose, but the question
>>> is if the change is acceptable during the stage4?
>>
>> Hmm, why don't we short-cut things earlier in execute_one_pass where
>> we handle TODO_discard_function?
>> That is, sth like
>>
>> Index: gcc/passes.c
>> ===================================================================
>> --- gcc/passes.c        (revision 234453)
>> +++ gcc/passes.c        (working copy)
>> @@ -2334,6 +2334,33 @@ execute_one_pass (opt_pass *pass)
>>
>>    /* Do it!  */
>>    todo_after = pass->execute (cfun);
>> +
>> +  if (todo_after & TODO_discard_function)
>> +    {
>> +      pass_fini_dump_file (pass);
>> +
>> +      gcc_assert (cfun);
>> +      /* As cgraph_node::release_body expects release dominators info,
>> +        we have to release it.  */
>> +      if (dom_info_available_p (CDI_DOMINATORS))
>> +       free_dominance_info (CDI_DOMINATORS);
>> +
>> +      if (dom_info_available_p (CDI_POST_DOMINATORS))
>> +       free_dominance_info (CDI_POST_DOMINATORS);
>> +
>> +      tree fn = cfun->decl;
>> +      pop_cfun ();
>> +      gcc_assert (!cfun);
>> +      cgraph_node::get (fn)->release_body ();
>> +
>> +      current_pass = NULL;
>> +      redirect_edge_var_map_empty ();
>> +
>> +      ggc_collect ();
>> +
>> +      return true;
>> +    }
>> +
>>    do_per_function (clear_last_verified, NULL);
>>
>>    /* Stop timevar.  */
>> @@ -2373,23 +2400,6 @@ execute_one_pass (opt_pass *pass)
>>    current_pass = NULL;
>>    redirect_edge_var_map_empty ();
>>
>> -  if (todo_after & TODO_discard_function)
>> -    {
>> -      gcc_assert (cfun);
>> -      /* As cgraph_node::release_body expects release dominators info,
>> -        we have to release it.  */
>> -      if (dom_info_available_p (CDI_DOMINATORS))
>> -       free_dominance_info (CDI_DOMINATORS);
>> -
>> -      if (dom_info_available_p (CDI_POST_DOMINATORS))
>> -       free_dominance_info (CDI_POST_DOMINATORS);
>> -
>> -      tree fn = cfun->decl;
>> -      pop_cfun ();
>> -      gcc_assert (!cfun);
>> -      cgraph_node::get (fn)->release_body ();
>> -    }
>> -
>>    /* Signal this is a suitable GC collection point.  */
>>    if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
>>      ggc_collect ();
>>
>>
>
> Hello Richi.
>
> Thanks for coming with the cleaner version of the patch.
> I've incorporated that patch and reg&bootstrap is running.
>
> Installable as soon as it finishes?

Yes.

Richard.

> Thanks,
> Martin
>
>>> Thanks,
>>> Martin
>

Patch
diff mbox

Index: gcc/passes.c
===================================================================
--- gcc/passes.c        (revision 234453)
+++ gcc/passes.c        (working copy)
@@ -2334,6 +2334,33 @@  execute_one_pass (opt_pass *pass)

   /* Do it!  */
   todo_after = pass->execute (cfun);
+
+  if (todo_after & TODO_discard_function)
+    {
+      pass_fini_dump_file (pass);
+
+      gcc_assert (cfun);
+      /* As cgraph_node::release_body expects release dominators info,
+        we have to release it.  */
+      if (dom_info_available_p (CDI_DOMINATORS))
+       free_dominance_info (CDI_DOMINATORS);
+
+      if (dom_info_available_p (CDI_POST_DOMINATORS))
+       free_dominance_info (CDI_POST_DOMINATORS);
+
+      tree fn = cfun->decl;
+      pop_cfun ();
+      gcc_assert (!cfun);
+      cgraph_node::get (fn)->release_body ();
+
+      current_pass = NULL;
+      redirect_edge_var_map_empty ();
+
+      ggc_collect ();
+
+      return true;
+    }
+
   do_per_function (clear_last_verified, NULL);

   /* Stop timevar.  */
@@ -2373,23 +2400,6 @@  execute_one_pass (opt_pass *pass)
   current_pass = NULL;
   redirect_edge_var_map_empty ();

-  if (todo_after & TODO_discard_function)
-    {
-      gcc_assert (cfun);
-      /* As cgraph_node::release_body expects release dominators info,
-        we have to release it.  */
-      if (dom_info_available_p (CDI_DOMINATORS))
-       free_dominance_info (CDI_DOMINATORS);
-
-      if (dom_info_available_p (CDI_POST_DOMINATORS))
-       free_dominance_info (CDI_POST_DOMINATORS);
-
-      tree fn = cfun->decl;
-      pop_cfun ();
-      gcc_assert (!cfun);
-      cgraph_node::get (fn)->release_body ();
-    }
-
   /* Signal this is a suitable GC collection point.  */
   if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
     ggc_collect ();