diff mbox

Fix PR 53743 and other -freorder-blocks-and-partition failures (issue6823047)

Message ID CABu31nMKMjNZvtJJwfimt-N8OUh2veEsGXdJ2UOFEigSZVNyGA@mail.gmail.com
State New
Headers show

Commit Message

Steven Bosscher Oct. 31, 2012, 11:02 p.m. UTC 
On Tue, Oct 30, 2012 at 10:43 PM, Teresa Johnson wrote:
> Sure, I will give this a try after your verification patch tests
> complete. Does this mean that the patch you posted above to
> force_nonfallthru_and_redirect is no longer needed either? I'll see if
> I can avoid the need for some of my fixes, although I believe at least
> the function.c one will still be needed. I'll check.

The force_nonfallthru change is still necessary, because
force_nonfallthru should be almost a no-op in cfglayout mode. The
whole concept of a fallthru edge doesn't exist in cfglayout mode: any
single_succ edge is a fallthru edge until the order of the basic
blocks has been determined and the insn chain is re-linked (cfglayout
mode originally was developed for bb-reorder, to move blocks around
more easily). So the correct patch would actually be:


(Or better yet: Remove the force_nonfallthru and tidy_fallthru_edge
hooks, they are cfgrtl-only.)

But obviously that won't work because
bb-reorder.c:fix_up_fall_thru_edges calls this hook while we're in
cfglayout mode. That is a bug. The call to force_nonfallthru results
in a "dangling" barrier:

cfgrtl.c:1523  emit_barrier_after (BB_END (jump_block));

In cfglayout mode, barriers don't exist in the insns chain, and they
don't have to because every edge is a fallthru edge. If there are
barriers before cfglayout mode, they are either removed or linked in
the basic block footer, and fixup_reorder_chain restores or inserts
barriers where necessary to drop out of cfglayout mode. This
emit_barrier_after call hangs a barrier after BB_END but not in the
footer, and I'm pretty sure the result will be that the barrier is
lost in fixup_reorder_chain. See also emit_barrier_after_bb for how
inserting a barrier should be done in cfglayout mode.

So in short, bbpart doesn't know what it wants to be: a cfgrtl or a
cfglayout pass. It doesn't work without cfglayout but it's doing
things that are only correct in the cfgrtl world and Very Wrong Indeed
in cfglayout-land.


> Regarding your earlier question about why we needed to add the
> barrier, I need to dig up the details again but essentially I found
> that the barriers were being added by bbpart, but bbro was reordering
> things and the block that ended up at the border between the hot and
> cold section didn't necessarily have a barrier on it because it was
> not previously at the region boundary.

That doesn't sound right. bbpart doesn't actually re-order the basic
blocks, it only marks the blocks with the partition they will be
assigned to. Whatever ends up at the border between the two partitions
is not relevant: the hot section cannot end in a fall-through edge to
the cold section (verify_flow_info even checks for that, see "fallthru
edge crosses section boundary (bb %i)") so it must end in some
explicit jump. Such jumps are always followed by a barrier. The only
reason I can think of why there might be a missing barrier, is because
fixup_reorder_chain has a bug and forgets to insert the barrier in
some cases (and I suspect this may be the case for return patterns, or
the a.m. issue of a dropper barrier).

I would like to work on debugging this, but it's hard without test cases...

Ciao!
Steven

Comments

Christophe Lyon Nov. 1, 2012, 3:53 p.m. UTC | #1 
> I would like to work on debugging this, but it's hard without test cases...
Maybe the files I attached to my PR55121 could help you in this respect?

Your "sanity checking" patching does complain with these input files.


Christophe.
diff mbox

Patch

Index: cfgrtl.c
===================================================================
--- cfgrtl.c    (revision 193046)
+++ cfgrtl.c    (working copy)
@@ -4547,7 +4547,7 @@  struct cfg_hooks cfg_layout_rtl_cfg_hooks = {
   cfg_layout_split_edge,
   rtl_make_forwarder_block,
   NULL, /* tidy_fallthru_edge */
-  rtl_force_nonfallthru,
+  NULL, /* force_nonfallthru */
   rtl_block_ends_with_call_p,
   rtl_block_ends_with_condjump_p,
   rtl_flow_call_edges_add,