PR16585: Fix setting/resetting of current_function_has_computed_jump

PR16585: Fix setting/resetting of current_function_has_computed_jump

[Steven Bosscher]

Hi,

The problem in this PR is that current_function_has_computed_jump is
cleared unconditionally in make_edges, but make_edges may be called
on a subchain of basic blocks.  If the computed jump is not in that
chain, we never properly set it again.

Whether current_function_has_computed_jump is set or not doesn't 
really affect the compilation much, but this issue is blocking the
updated patch I have for the more serious problem in PR15242.

Does this approach look sane, or should I perhaps move this code
to cfgcleanup.c?

Gr.
Steven


	* cfgbuild.c (make_edges): Don't unconditionally clear
	current_function_has_computed_jump.  Look through all blocks
	if it was set before, to see if there still is a computed
	jump ending any basic block.

Index: cfgbuild.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cfgbuild.c,v
retrieving revision 1.55
diff -c -3 -p -r1.55 cfgbuild.c
*** cfgbuild.c	28 Sep 2004 07:59:42 -0000	1.55
--- cfgbuild.c	25 Oct 2004 21:34:20 -0000
*************** static void
*** 225,244 ****
  make_edges (basic_block min, basic_block max, int update_p)
  {
    basic_block bb;
    sbitmap *edge_cache = NULL;
  
-   /* Assume no computed jump; revise as we create edges.  */
-   current_function_has_computed_jump = 0;
- 
-   /* If we are partitioning hot and cold basic blocks into separate
-      sections, we cannot assume there is no computed jump (partitioning
-      sometimes requires the use of indirect jumps; see comments about
-      partitioning at the top of bb-reorder.c:partition_hot_cold_basic_blocks 
-      for complete details).  */
- 
-   if (flag_reorder_blocks_and_partition)
-     current_function_has_computed_jump = 1;
- 
    /* Heavy use of computed goto in machine-generated code can lead to
       nearly fully-connected CFGs.  In that case we spend a significant
       amount of time searching the edge lists for duplicates.  */
--- 225,233 ----
  make_edges (basic_block min, basic_block max, int update_p)
  {
    basic_block bb;
+   bool seen_computed_jump = false;
    sbitmap *edge_cache = NULL;
  
    /* Heavy use of computed goto in machine-generated code can lead to
       nearly fully-connected CFGs.  In that case we spend a significant
       amount of time searching the edge lists for duplicates.  */
*************** make_edges (basic_block min, basic_block
*** 333,339 ****
  	     everything on the forced_labels list.  */
  	  else if (computed_jump_p (insn))
  	    {
! 	      current_function_has_computed_jump = 1;
  
  	      for (x = forced_labels; x; x = XEXP (x, 1))
  		make_label_edge (edge_cache, bb, XEXP (x, 0), EDGE_ABNORMAL);
--- 322,328 ----
  	     everything on the forced_labels list.  */
  	  else if (computed_jump_p (insn))
  	    {
! 	      seen_computed_jump = true;
  
  	      for (x = forced_labels; x; x = XEXP (x, 1))
  		make_label_edge (edge_cache, bb, XEXP (x, 0), EDGE_ABNORMAL);
*************** make_edges (basic_block min, basic_block
*** 412,417 ****
--- 401,437 ----
  
    if (edge_cache)
      sbitmap_vector_free (edge_cache);
+ 
+   /* If we have seen a computed jump, current_function_has_computed_jump
+      should be set.
+      If we are partitioning hot and cold basic blocks into separate
+      sections, we cannot assume there is no computed jump (partitioning
+      sometimes requires the use of indirect jumps; see comments about
+      partitioning at the top of bb-reorder.c:partition_hot_cold_basic_blocks 
+      for complete details).  */
+   if (seen_computed_jump || flag_reorder_blocks_and_partition)
+     current_function_has_computed_jump = 1;
+ 
+   /* Otherwise if we've previously seen computed jumps, go through all
+      basic blocks and see if we find one that ends in a computed jump
+      to confirm that it is still there.  If it is not, we can safely
+      clear current_function_has_computed_jump.
+      Only do this if flag_expensive_optimizations is set, because this
+      is a relatively expensive thing to do - computed jumps are almost
+      never optimized away.  */
+   else if (current_function_has_computed_jump
+ 	   && !seen_computed_jump
+ 	   && flag_expensive_optimizations)
+     {
+       FOR_EACH_BB (bb)
+ 	if (computed_jump_p (BB_END (bb)))
+ 	  {
+ 	    seen_computed_jump = true;
+ 	    break;
+ 	  }
+       if (!seen_computed_jump)
+ 	current_function_has_computed_jump = 0;
+     }
  }
  
  /* Find all basic blocks of the function whose first insn is F.
Index: tree-cfg.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-cfg.c,v
retrieving revision 2.89
diff -c -3 -p -r2.89 tree-cfg.c
*** tree-cfg.c	23 Oct 2004 19:17:08 -0000	2.89
--- tree-cfg.c	25 Oct 2004 21:34:21 -0000
*************** build_tree_cfg (tree *tp)
*** 148,154 ****
       edge list.  After we convert back to normal form, we will un-factor
       the computed gotos since factoring introduces an unwanted jump.  */
    if (found_computed_goto)
!     factor_computed_gotos ();
  
    /* Make sure there is always at least one block, even if it's empty.  */
    if (n_basic_blocks == 0)
--- 148,157 ----
       edge list.  After we convert back to normal form, we will un-factor
       the computed gotos since factoring introduces an unwanted jump.  */
    if (found_computed_goto)
!     {
!       factor_computed_gotos ();
!       current_function_has_computed_jump = 1;
!     }
  
    /* Make sure there is always at least one block, even if it's empty.  */
    if (n_basic_blocks == 0)

Re: PR16585: Fix setting/resetting of current_function_has_computed_jump

[Richard Henderson]

On Mon, Oct 25, 2004 at 11:36:23PM +0200, Steven Bosscher wrote:
> Does this approach look sane, or should I perhaps move this code
> to cfgcleanup.c?

This doesn't look like the best fix.  I think that at minimum,
the _clearing_ of current_function_has_computed_jump should be
moved somewhere else.  It might be best to still set it here,
but I'm not sure.


r~

Re: PR16585: Fix setting/resetting of current_function_has_computed_jump

[Steven Bosscher]

On Tuesday 26 October 2004 00:13, Richard Henderson wrote:
> On Mon, Oct 25, 2004 at 11:36:23PM +0200, Steven Bosscher wrote:
> > Does this approach look sane, or should I perhaps move this code
> > to cfgcleanup.c?
>
> This doesn't look like the best fix.  I think that at minimum,
> the _clearing_ of current_function_has_computed_jump should be
> moved somewhere else.  It might be best to still set it here,
> but I'm not sure.

Yeah, I thought it was an odd place to have this, too.  In fact
I was thinking perhaps we should just not know about this
current_function_has_computed_jump thing until much later in
the compilation.  The only pass that actually cares about this
is sched-rgn.

Gr.
Steven

Re: PR16585: Fix setting/resetting of current_function_has_computed_jump

[Richard Henderson]

On Tue, Oct 26, 2004 at 12:18:29AM +0200, Steven Bosscher wrote:
> Yeah, I thought it was an odd place to have this, too.  In fact
> I was thinking perhaps we should just not know about this
> current_function_has_computed_jump thing until much later in
> the compilation.  The only pass that actually cares about this
> is sched-rgn.

That would work for me.


r~