Improve code quality across partition boundaries

Hi,
this patch fixes most offending artifacts across the function partition
bounary with -freorder-blocks-and-partitions which is now on by default
for i386 (no other targets).  The problem is that most of cfgcleanup and
cfgrtl is disabled on crossing edges and thus we produce pretty ugly
code.

The comment I removed reffers to non-existing comment, but the fixup
of partitioning only looks for crossing jumps. It generates new
patterns for unconditional jumps and ads trampolines for conditional
jumps if taret requests it (i386 doesn't).

It is thus safe to turn any far jump to near jump (as this patch does)
and based on target we can do more (this patch doesn't)

For this late of stage4 I think we are fine with enabling edge forwarding
only. This saves about 0.5% of code segment on cc1 (measured with and
without patch so the code pays back at least) with profiledbootstrap.
I have patches to fix other issues but I think they can wait for next
stage1.  In some cases we would get better code with crossjumping
too, but it does not seem critical.

profiledbootstrapped/regtested x86_64, ltoprofiledbootstrapped x86_64 and also
profiledbootstrapped on power. I am running firefox build with LTO+FDO on
x86_64 too and regtesting on power. OK?

Honza

	PR rtl/84058
	* cfgcleanup.c (try_forward_edges): Do not give up on crossing
	jumps; choose last target that matches the criteria (i.e.
	no partition changes for non-crossing jumps).
	* cfgrtl.c (cfg_layout_redirect_edge_and_branch): Add basic
	support for redirecting crossing jumps to non-crossing.

On April 9, 2018 4:17:45 PM GMT+02:00, Jan Hubicka <hubicka@ucw.cz> wrote:
>Hi,
>this patch fixes most offending artifacts across the function partition
>bounary with -freorder-blocks-and-partitions which is now on by default
>for i386 (no other targets).  The problem is that most of cfgcleanup
>and
>cfgrtl is disabled on crossing edges and thus we produce pretty ugly
>code.
>
>The comment I removed reffers to non-existing comment, but the fixup
>of partitioning only looks for crossing jumps. It generates new
>patterns for unconditional jumps and ads trampolines for conditional
>jumps if taret requests it (i386 doesn't).
>
>It is thus safe to turn any far jump to near jump (as this patch does)
>and based on target we can do more (this patch doesn't)
>
>For this late of stage4 I think we are fine with enabling edge
>forwarding
>only. This saves about 0.5% of code segment on cc1 (measured with and
>without patch so the code pays back at least) with profiledbootstrap.
>I have patches to fix other issues but I think they can wait for next
>stage1.  In some cases we would get better code with crossjumping
>too, but it does not seem critical.
>
>profiledbootstrapped/regtested x86_64, ltoprofiledbootstrapped x86_64
>and also
>profiledbootstrapped on power. I am running firefox build with LTO+FDO
>on
>x86_64 too and regtesting on power. OK?

OK. 

Richard. 

>Honza
>
>	PR rtl/84058
>	* cfgcleanup.c (try_forward_edges): Do not give up on crossing
>	jumps; choose last target that matches the criteria (i.e.
>	no partition changes for non-crossing jumps).
>	* cfgrtl.c (cfg_layout_redirect_edge_and_branch): Add basic
>	support for redirecting crossing jumps to non-crossing.
>Index: cfgcleanup.c
>===================================================================
>--- cfgcleanup.c	(revision 259167)
>+++ cfgcleanup.c	(working copy)
>@@ -394,19 +394,6 @@
>   edge_iterator ei;
>   edge e, *threaded_edges = NULL;
> 
>-  /* If we are partitioning hot/cold basic blocks, we don't want to
>-     mess up unconditional or indirect jumps that cross between hot
>-     and cold sections.
>-
>-     Basic block partitioning may result in some jumps that appear to
>-     be optimizable (or blocks that appear to be mergeable), but which
>really
>-     must be left untouched (they are required to make it safely
>across
>-     partition boundaries).  See the comments at the top of
>-     bb-reorder.c:partition_hot_cold_basic_blocks for complete
>details.  */
>-
>-  if (JUMP_P (BB_END (b)) && CROSSING_JUMP_P (BB_END (b)))
>-    return false;
>-
>   for (ei = ei_start (b->succs); (e = ei_safe_edge (ei)); )
>     {
>       basic_block target, first;
>@@ -415,6 +402,7 @@
>       bool threaded = false;
>       int nthreaded_edges = 0;
>       bool may_thread = first_pass || (b->flags & BB_MODIFIED) != 0;
>+      bool new_target_threaded = false;
> 
>       /* Skip complex edges because we don't know how to update them.
> 
>@@ -431,29 +419,12 @@
>       counter = NUM_FIXED_BLOCKS;
>       goto_locus = e->goto_locus;
> 
>-      /* If we are partitioning hot/cold basic_blocks, we don't want
>to mess
>-	 up jumps that cross between hot/cold sections.
>-
>-	 Basic block partitioning may result in some jumps that appear
>-	 to be optimizable (or blocks that appear to be mergeable), but which
>-	 really must be left untouched (they are required to make it safely
>-	 across partition boundaries).  See the comments at the top of
>-	 bb-reorder.c:partition_hot_cold_basic_blocks for complete
>-	 details.  */
>-
>-      if (first != EXIT_BLOCK_PTR_FOR_FN (cfun)
>-	  && JUMP_P (BB_END (first))
>-	  && CROSSING_JUMP_P (BB_END (first)))
>-	return changed;
>-
>       while (counter < n_basic_blocks_for_fn (cfun))
> 	{
> 	  basic_block new_target = NULL;
>-	  bool new_target_threaded = false;
> 	  may_thread |= (target->flags & BB_MODIFIED) != 0;
> 
> 	  if (FORWARDER_BLOCK_P (target)
>-	      && !(single_succ_edge (target)->flags & EDGE_CROSSING)
> 	      && single_succ (target) != EXIT_BLOCK_PTR_FOR_FN (cfun))
> 	    {
> 	      /* Bypass trivial infinite loops.  */
>@@ -543,8 +514,14 @@
> 	    break;
> 
> 	  counter++;
>-	  target = new_target;
>-	  threaded |= new_target_threaded;
>+	  /* Do not turn non-crossing jump to crossing.  Depending on target
>+	     it may require different instruction pattern.  */
>+	  if ((e->flags & EDGE_CROSSING)
>+	      || BB_PARTITION (first) == BB_PARTITION (new_target))
>+	    {
>+	      target = new_target;
>+	      threaded |= new_target_threaded;
>+	    }
> 	}
> 
>       if (counter >= n_basic_blocks_for_fn (cfun))
>Index: cfgrtl.c
>===================================================================
>--- cfgrtl.c	(revision 259167)
>+++ cfgrtl.c	(working copy)
>@@ -4361,6 +4361,18 @@
>   if (e->dest == dest)
>     return e;
> 
>+  if (e->flags & EDGE_CROSSING
>+      && BB_PARTITION (e->src) == BB_PARTITION (dest)
>+      && simplejump_p (BB_END (src)))
>+    {
>+      if (dump_file)
>+	fprintf (dump_file,
>+	  	 "Removing crossing jump while redirecting edge form %i to %i\n",
>+		 e->src->index, dest->index);
>+      delete_insn (BB_END (src));
>+      e->flags |= EDGE_FALLTHRU;
>+    }
>+
>   if (e->src != ENTRY_BLOCK_PTR_FOR_FN (cfun)
>       && (ret = try_redirect_by_replacing_jump (e, dest, true)))
>     {
>@@ -4424,8 +4436,9 @@
>   else
>     ret = redirect_branch_edge (e, dest);
> 
>+  fixup_partition_crossing (ret);
>  /* We don't want simplejumps in the insn stream during cfglayout.  */
>-  gcc_assert (!simplejump_p (BB_END (src)));
>+  gcc_assert (!simplejump_p (BB_END (src)) || CROSSING_JUMP_P (BB_END
>(src)));
> 
>   df_set_bb_dirty (src);
>   return ret;