diff mbox

Fix epilogue bb expansion (PR middle-end/60175)

Message ID 20140217194528.GH22862@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Feb. 17, 2014, 7:45 p.m. UTC 
Hi!

On ARM we ICE on nosanitize-and-inline.c testcase during loop unrolling,
but the bug seems to be on all targets that during expansion the frequency
of the pre-exit basic block is out of bounds.

The problem is that we have a function ending with return 0;, so
that basic block is expanded as unconditional jump to the return_label.
When construct_exit_block is called, expand_function_end emits some clobbers
for the case when a function has fallthru into the exit block, and only
after that return_label to which the unconditional jump jumps.

frequency of the new exit block (bb4) is set from the frequency of the EXIT
block (bb1), but then when find_many_sub_basic_blocks is called, it of
course needs to create a new basic block at return_label, because it is a
label that should start a basic block.  But that leaves us with a 10000
frequency basic block falling through to the one with return_label, and
unconditional jump from a bb with frequency 10000 to that basic block too,
so frequency 20000 instead of maximum 10000.

Without -fsanitize=address this isn't normally a problem because cfg cleanup
merges the new basic block with the preceeding one and thus the frequency is
ignored.  But -fsanitize=address needs to do conditional branch in the exit
block and that leads to leaking the out of bound frequencies like 19992
into the IL.

Fixed by not adding clobbers at all after BARRIER (at that point we know
there will not be a fallthru, so why should we emit something that is going
to be removed immediately anyway), and by appending anything that is emitted
before the return_label to the prev_bb basic block. 
find_many_sub_basic_blocks should take care of splitting it if there is no
fallthru without control insn.

Initially I had in that code (if there is anything other than return_label)
at head an assert that find_fallthru_edge (prev_bb->succs) is non-NULL,
but turns out that in some cases e.g. prev_bb ends with a __builtin_trap ()
and expand_function_end emitted a pending stack adjustment into the sequence
(which of course should be thrown away afterwards), so I've removed the
assert; find_many_sub_basic_blocks/cfg cleanup takes care of throwing away
the dead insns in that case.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2014-02-17  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/60175
	* function.c (expand_function_end): Don't emit
	clobber_return_register sequence if clobber_after is a BARRIER.
	* cfgexpand.c (construct_exit_block): Append instructions before
	return_label to prev_bb.


	Jakub

Comments

Richard Henderson Feb. 28, 2014, 4:32 p.m. UTC | #1 
On 02/17/2014 11:45 AM, Jakub Jelinek wrote:
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2014-02-17  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/60175
> 	* function.c (expand_function_end): Don't emit
> 	clobber_return_register sequence if clobber_after is a BARRIER.
> 	* cfgexpand.c (construct_exit_block): Append instructions before
> 	return_label to prev_bb.

Ok.


r~
diff mbox

Patch

--- gcc/function.c.jj	2014-02-17 10:05:24.720195450 +0100
+++ gcc/function.c	2014-02-17 11:20:35.149587876 +0100
@@ -5156,17 +5156,20 @@  expand_function_end (void)
       crtl->return_rtx = outgoing;
     }
 
-  /* Emit the actual code to clobber return register.  */
-  {
-    rtx seq;
+  /* Emit the actual code to clobber return register.  Don't emit
+     it if clobber_after is a barrier, then the previous basic block
+     certainly doesn't fall thru into the exit block.  */
+  if (!BARRIER_P (clobber_after))
+    {
+      rtx seq;
 
-    start_sequence ();
-    clobber_return_register ();
-    seq = get_insns ();
-    end_sequence ();
+      start_sequence ();
+      clobber_return_register ();
+      seq = get_insns ();
+      end_sequence ();
 
-    emit_insn_after (seq, clobber_after);
-  }
+      emit_insn_after (seq, clobber_after);
+    }
 
   /* Output the label for the naked return from the function.  */
   if (naked_return_label)
--- gcc/cfgexpand.c.jj	2014-02-11 09:58:19.000000000 +0100
+++ gcc/cfgexpand.c	2014-02-17 12:14:44.441191129 +0100
@@ -5273,7 +5273,8 @@  construct_exit_block (void)
   edge e, e2;
   unsigned ix;
   edge_iterator ei;
-  rtx orig_end = BB_END (EXIT_BLOCK_PTR_FOR_FN (cfun)->prev_bb);
+  basic_block prev_bb = EXIT_BLOCK_PTR_FOR_FN (cfun)->prev_bb;
+  rtx orig_end = BB_END (prev_bb);
 
   rtl_profile_for_bb (EXIT_BLOCK_PTR_FOR_FN (cfun));
 
@@ -5288,13 +5289,25 @@  construct_exit_block (void)
   end = get_last_insn ();
   if (head == end)
     return;
-  /* While emitting the function end we could move end of the last basic block.
-   */
-  BB_END (EXIT_BLOCK_PTR_FOR_FN (cfun)->prev_bb) = orig_end;
+  /* While emitting the function end we could move end of the last basic
+     block.  */
+  BB_END (prev_bb) = orig_end;
   while (NEXT_INSN (head) && NOTE_P (NEXT_INSN (head)))
     head = NEXT_INSN (head);
-  exit_block = create_basic_block (NEXT_INSN (head), end,
-				   EXIT_BLOCK_PTR_FOR_FN (cfun)->prev_bb);
+  /* But make sure exit_block starts with RETURN_LABEL, otherwise the
+     bb frequency counting will be confused.  Any instructions before that
+     label are emitted for the case where PREV_BB falls through into the
+     exit block, so append those instructions to prev_bb in that case.  */
+  if (NEXT_INSN (head) != return_label)
+    {
+      while (NEXT_INSN (head) != return_label)
+	{
+	  if (!NOTE_P (NEXT_INSN (head)))
+	    BB_END (prev_bb) = NEXT_INSN (head);
+	  head = NEXT_INSN (head);
+	}
+    }
+  exit_block = create_basic_block (NEXT_INSN (head), end, prev_bb);
   exit_block->frequency = EXIT_BLOCK_PTR_FOR_FN (cfun)->frequency;
   exit_block->count = EXIT_BLOCK_PTR_FOR_FN (cfun)->count;
   if (current_loops && EXIT_BLOCK_PTR_FOR_FN (cfun)->loop_father)