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:
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,
(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
