diff mbox

[RFA] Switch cgraph to IPA_SSA state reliably

Message ID 4C3F5B72.3030201@redhat.com
State New
Headers show

Commit Message

Richard Henderson July 15, 2010, 7:03 p.m. UTC 
This is a piece of the emutls rewrite.

The ipa_lower_emutls pass is placed at the end of all_small_ipa_passes.  This
position allows the OMP and profile passes within pass_early_local_passes
to create new TLS variables without having to worry about emutls at all.

The ipa_lower_emutls pass may create one new function: a constructor to 
perform the runtime initialization of any DECL_COMMON tls variables.  Creating
this constructor during the IPA pass is easier and tidier than the existing
method of creating it at the end of compilation.

However, this exposes a problem with cgraph_state: it isn't updated properly.
When ipa_lower_emutls runs, cgraph_state == IPA but not IPA_SSA, despite the
fact that all functions *are* in SSA form.  Further, the new function is never
placed in SSA form by cgraph_process_new_functions, so when we get to
pass_all_optimizations we crash due to SSA state not being allocated.

I'm not sure of the logic behind switching cgraph_state in pass_all_early_optimizations.
For whatever reason, it's not working.  Simply moving the state change to
pass_early_local_passes does the trick, however.

Bootstrap is continuing on amd64-linux.  Ok for mainline if it passes?


r~
* cgraphunit.c (ipa_passes): Assert we're already in IPA_SSA state,
	rather than forcing it.
	* tree-optimize.c (execute_early_local_optimizations): Remove.
	(execute_all_early_local_passes): Add.  Adjust passes to match.

Comments

Richard Henderson July 15, 2010, 7:44 p.m. UTC | #1 
On 07/15/2010 12:03 PM, Richard Henderson wrote:
> I'm not sure of the logic behind switching cgraph_state in pass_all_early_optimizations.
> For whatever reason, it's not working.  Simply moving the state change to
> pass_early_local_passes does the trick, however.

I figured out why the state change in pass_all_early_optimizations wasn't working:
it's not run all the time.  Consider a compilation unit with no functions.  E.g.

  __thread int i __attribute__((common));

This test case requires a constructor, but since pass_all_early_optimizations
is run for each function and there are none, we never change state to IPA_SSA.

> Bootstrap is continuing on amd64-linux.  Ok for mainline if it passes?

Testing failed.

> -  /* If pass_all_early_optimizations was not scheduled, the state of
> -     the cgraph will not be properly updated.  Update it now.  */
> -  if (cgraph_state < CGRAPH_STATE_IPA_SSA)
> -    cgraph_state = CGRAPH_STATE_IPA_SSA;
> +  /* We should have run pass_early_local_passes, which updated this state.  */
> +  gcc_assert (cgraph_state >= CGRAPH_STATE_IPA_SSA);

Ignore this hunk for the moment; it fails for all LTO tests.  However, I suspect
that somewhere in the LTO path we should have set the state more correctly.


r~
Richard Henderson July 15, 2010, 9:42 p.m. UTC | #2 
On 07/15/2010 12:44 PM, Richard Henderson wrote:
>> -  /* If pass_all_early_optimizations was not scheduled, the state of
>> -     the cgraph will not be properly updated.  Update it now.  */
>> -  if (cgraph_state < CGRAPH_STATE_IPA_SSA)
>> -    cgraph_state = CGRAPH_STATE_IPA_SSA;
>> +  /* We should have run pass_early_local_passes, which updated this state.  */
>> +  gcc_assert (cgraph_state >= CGRAPH_STATE_IPA_SSA);
> 
> Ignore this hunk for the moment; it fails for all LTO tests.  However, I suspect
> that somewhere in the LTO path we should have set the state more correctly.

Bootstrap and test on x86_64-linux succeeds without this hunk.


r~
Richard Henderson July 19, 2010, 2:43 p.m. UTC | #3 
On 07/15/2010 02:42 PM, Richard Henderson wrote:
> On 07/15/2010 12:44 PM, Richard Henderson wrote:
>>> -  /* If pass_all_early_optimizations was not scheduled, the state of
>>> -     the cgraph will not be properly updated.  Update it now.  */
>>> -  if (cgraph_state < CGRAPH_STATE_IPA_SSA)
>>> -    cgraph_state = CGRAPH_STATE_IPA_SSA;
>>> +  /* We should have run pass_early_local_passes, which updated this state.  */
>>> +  gcc_assert (cgraph_state >= CGRAPH_STATE_IPA_SSA);
>>
>> Ignore this hunk for the moment; it fails for all LTO tests.  However, I suspect
>> that somewhere in the LTO path we should have set the state more correctly.
> 
> Bootstrap and test on x86_64-linux succeeds without this hunk.

I committed the patch.



r~
diff mbox

Patch

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 47f8f76..cf58a75 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1850,10 +1850,8 @@  ipa_passes (void)
   if (!in_lto_p)
     execute_ipa_pass_list (all_small_ipa_passes);
 
-  /* If pass_all_early_optimizations was not scheduled, the state of
-     the cgraph will not be properly updated.  Update it now.  */
-  if (cgraph_state < CGRAPH_STATE_IPA_SSA)
-    cgraph_state = CGRAPH_STATE_IPA_SSA;
+  /* We should have run pass_early_local_passes, which updated this state.  */
+  gcc_assert (cgraph_state >= CGRAPH_STATE_IPA_SSA);
 
   if (!in_lto_p)
     {
diff --git a/gcc/tree-optimize.c b/gcc/tree-optimize.c
index e736b4f..5df3fdb 100644
--- a/gcc/tree-optimize.c
+++ b/gcc/tree-optimize.c
@@ -87,13 +87,27 @@  gate_all_early_local_passes (void)
   return (!seen_error () && !in_lto_p);
 }
 
+static unsigned int
+execute_all_early_local_passes (void)
+{
+  /* Once this pass (and its sub-passes) are complete, all functions
+     will be in SSA form.  Technically this state change is happening
+     a tad early, since the sub-passes have not yet run, but since
+     none of the sub-passes are IPA passes and do not create new
+     functions, this is ok.  We're setting this value for the benefit
+     of IPA passes that follow.  */
+  if (cgraph_state < CGRAPH_STATE_IPA_SSA)
+    cgraph_state = CGRAPH_STATE_IPA_SSA;
+  return 0;
+}
+
 struct simple_ipa_opt_pass pass_early_local_passes =
 {
  {
   SIMPLE_IPA_PASS,
   "early_local_cleanups",		/* name */
   gate_all_early_local_passes,		/* gate */
-  NULL,					/* execute */
+  execute_all_early_local_passes,	/* execute */
   NULL,					/* sub */
   NULL,					/* next */
   0,					/* static_pass_number */
@@ -106,18 +120,6 @@  struct simple_ipa_opt_pass pass_early_local_passes =
  }
 };
 
-static unsigned int
-execute_early_local_optimizations (void)
-{
-  /* First time we start with early optimization we need to advance
-     cgraph state so newly inserted functions are also early optimized.
-     However we execute early local optimizations for lately inserted
-     functions, in that case don't reset cgraph state back to IPA_SSA.  */
-  if (cgraph_state < CGRAPH_STATE_IPA_SSA)
-    cgraph_state = CGRAPH_STATE_IPA_SSA;
-  return 0;
-}
-
 /* Gate: execute, or not, all of the non-trivial optimizations.  */
 
 static bool
@@ -134,7 +136,7 @@  struct gimple_opt_pass pass_all_early_optimizations =
   GIMPLE_PASS,
   "early_optimizations",		/* name */
   gate_all_early_optimizations,		/* gate */
-  execute_early_local_optimizations,	/* execute */
+  NULL,					/* execute */
   NULL,					/* sub */
   NULL,					/* next */
   0,					/* static_pass_number */