diff mbox

Try to update dominance info in tree-call-cdce.c

Message ID 87d1vsv1n2.fsf@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford Nov. 2, 2015, 3:53 p.m. UTC 
Steven Bosscher <stevenb.gcc@gmail.com> writes:
> On Fri, Oct 30, 2015 at 2:11 PM, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>> Is the split_block change really so bad?
>
> IMHO: Yes.

Fair enough :-)

> split_block just splits some basic block B into two blocks B1/B2
> somewhere in the middle of B. The dominance relations between B1 and
> B2 are obvious and intuitive. The new flag to split_block is not. The
> dominance info is not updated but the loops are? Confusing, if you ask
> me...
>
> Your situation, where a pass knows the dominance relations will
> change, is unusual. The extra work to fix up the ET tree twice should
> be negligible anyway.

I just thought that the cfg was in practice never going to stay in the
state that it is on return from split_block.  The caller is bound to
add (at least) another edge somewhere.

Here's a version that works with split_block rather than against it.
Tested on x86_64-linux-gnu, arm-linux-gnueabi and aarch64-linux-gnu.
OK to install?

Thanks,
Richard

    
gcc/
    	* tree-call-cdce.c (shrink_wrap_one_built_in_call): Try to update
    	the dominance info; free it if we can't.
    	(pass_call_cdce::execute): Don't free the dominance info here.

Comments

Jeff Law Nov. 2, 2015, 10:25 p.m. UTC | #1 
On 11/02/2015 08:53 AM, Richard Sandiford wrote:
> Steven Bosscher <stevenb.gcc@gmail.com> writes:
>> On Fri, Oct 30, 2015 at 2:11 PM, Richard Sandiford
>> <richard.sandiford@arm.com> wrote:
>>> Is the split_block change really so bad?
>>
>> IMHO: Yes.
>
> Fair enough :-)
I tend to agree.  If the caller needs a more complex control flow and 
dominance/post-dominance relationship set up, then it seems like it 
belongs in the caller.

If there's enough commonality in cases where the caller is going to do 
something more complex with the CFG, then perhaps there could be a 
function to (as Richi suggests) create diamonds, half diamonds or 
something else commonly needed.

>
> I just thought that the cfg was in practice never going to stay in the
> state that it is on return from split_block.  The caller is bound to
> add (at least) another edge somewhere.
Perhaps.  But that functionality (assuming it's common) can be layered 
on top of split_block pretty easily it seems.


>
> Here's a version that works with split_block rather than against it.
> Tested on x86_64-linux-gnu, arm-linux-gnueabi and aarch64-linux-gnu.
> OK to install?
>
> Thanks,
> Richard
>
>
> gcc/
>      	* tree-call-cdce.c (shrink_wrap_one_built_in_call): Try to update
>      	the dominance info; free it if we can't.
>      	(pass_call_cdce::execute): Don't free the dominance info here.
OK.

Thoughts on emitting something into the dump file in tree-call-cdce.c 
which indicates when the dominators/post-dominators are invalidated?  Or 
better yet, put it in free_dominance_info.  Then we could have a 
testcase which verifies that we're updating rather than recomputing in 
the case which led to this change, and in cases where there's EH bits to 
update that we recompute.

Jeff
diff mbox

Patch

diff --git a/gcc/tree-call-cdce.c b/gcc/tree-call-cdce.c
index ffc1c4e..a5f38ce 100644
--- a/gcc/tree-call-cdce.c
+++ b/gcc/tree-call-cdce.c
@@ -731,6 +731,32 @@  shrink_wrap_one_built_in_call (gcall *bi_call)
   if (nconds == 0)
     return false;
 
+  /* The cfg we want to create looks like this:
+
+	   [guard n-1]         <- guard_bb (old block)
+	     |    \
+	     | [guard n-2]                   }
+	     |    / \                        }
+	     |   /  ...                      } new blocks
+	     |  /  [guard 0]                 }
+	     | /    /   |                    }
+	    [ call ]    |     <- bi_call_bb  }
+	     | \        |
+	     |  \       |
+	     |   [ join ]     <- join_tgt_bb (old iff call must end bb)
+	     |
+	 possible EH edges (only if [join] is old)
+
+     When [join] is new, the immediate dominators for these blocks are:
+
+     1. [guard n-1]: unchanged
+     2. [call]: [guard n-1]
+     3. [guard m]: [guard m+1] for 0 <= m <= n-2
+     4. [join]: [guard n-1]
+
+     We punt for the more complex case case of [join] being old and
+     simply free the dominance info.  We also punt on postdominators,
+     which aren't expected to be available at this point anyway.  */
   bi_call_bb = gimple_bb (bi_call);
 
   /* Now find the join target bb -- split bi_call_bb if needed.  */
@@ -741,6 +767,7 @@  shrink_wrap_one_built_in_call (gcall *bi_call)
       join_tgt_in_edge_from_call = find_fallthru_edge (bi_call_bb->succs);
       if (join_tgt_in_edge_from_call == NULL)
         return false;
+      free_dominance_info (CDI_DOMINATORS);
     }
   else
     join_tgt_in_edge_from_call = split_block (bi_call_bb, bi_call);
@@ -820,6 +847,15 @@  shrink_wrap_one_built_in_call (gcall *bi_call)
       guard_bb_in_edge->count = guard_bb->count - bi_call_in_edge->count;
     }
 
+  if (dom_info_available_p (CDI_DOMINATORS))
+    {
+      /* The split_blocks leave [guard 0] as the immediate dominator
+	 of [call] and [call] as the immediate dominator of [join].
+	 Fix them up.  */
+      set_immediate_dominator (CDI_DOMINATORS, bi_call_bb, guard_bb);
+      set_immediate_dominator (CDI_DOMINATORS, join_tgt_bb, guard_bb);
+    }
+
   if (dump_file && (dump_flags & TDF_DETAILS))
     {
       location_t loc;
@@ -927,7 +963,6 @@  pass_call_cdce::execute (function *fun)
 
   if (something_changed)
     {
-      free_dominance_info (CDI_DOMINATORS);
       free_dominance_info (CDI_POST_DOMINATORS);
       /* As we introduced new control-flow we need to insert PHI-nodes
          for the call-clobbers of the remaining call.  */