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

[Steven Bosscher <stevenb dot gcc at gmail dot com>]

On Sat, May 11, 2013 at 5:21 AM, Teresa Johnson wrote:
> Here there was a block that happened to be laid out at the very start
> of the cold section (it was jumped to from elsewhere, not reached via
> fall through from its layout predecessor). Thus it was preceded by a
> switch section note, which was put into the bb header when we entered
> cfglayout mode for compgoto. The note ended up in the middle of the
> block when we did some block combining with its cfg predecessor (not
> the block that preceded it in the layout chain, which was the last hot
> block in the reorder chain).

Yikes. So we also need an INSN_NOTE verifier to make sure that kind of
non-sense doesn't happen? More reason to make the note go away :-)

(See my recent patch that added note_outside_basic_block_p for other
examples of NOTEs ending up where they don't belong. It's one of those
most-people-want-it-but-nobody-does-it tasks: To create some frame
work for NOTE_INSN_* notes that is safe and sane...).


>> Please make the note go away completely before pass_free_cfg, and earn
>> greater admiration than Zeus. The note always was wrong, and now
>> you've shown it's also a problem.

Many, many thanks!

>> Right, I think it's good that you've centralized this code. But it
>> seems to me that we should make sure that the hot blocks and cold
>> blocks are still grouped (i.e. there is only one basic block B such
>> that BB_PARTITION(B) != BB_PARTITION(B->next_bb). That is something
>> your code doesn't handle, AFAICT. It's just one thing that's difficult
>> to maintain, probably there are others. It's also something the
>> partitioning verifier should check, i.e. that the basic blocks in hot
>> and cold partitions are properly grouped.
>
> Actually, there is already code that verifies this at the end of bbro
> (verify_hot_cold_block_grouping()). Before bb reordering it doesn't
> make sense to check this.
>
> And AFAICT, after bbro the only place we go into and out of cfglayout
> mode is compgoto, which duplicates blocks along edges only if they
> don't cross a partition boundary, and lays out the duplicated block
> adjacent to the original. I haven't seen any places where this is
> violated, probably as a result. But it wouldn't be a bad idea to call
> verify_hot_cold_block_grouping again during the flow verification code
> once we detect/flag that bbro is complete.

I'd just make it part of verify_flow_info and only let it run if your
new flag on crtl is set.

You shouldn't count on compgoto being the last and only pass to go
into/out-of cfglayout mode. The compiler changes all the time, passes
get shuffled around from time to time, and target-specific passes can
be inserted anywhere. I have patches in my queue to lengthen the life
of the CFG beyond pass_machine_reorg (many machine reorgs are CFG
aware already anyway) and they should be allowed to go into/out-of
cfglayout mode also.



>>> +  /* Invoke the cleanup again once we are out of cfg layout mode
>>> +     after committing the final bb layout above. This enables removal
>>> +     of forwarding blocks across the hot/cold section boundary when
>>> +     splitting is enabled that were necessary while in cfg layout
>>> +     mode.  */
>>> +  if (crtl->has_bb_partition)
>>> +    cleanup_cfg (CLEANUP_EXPENSIVE);
>>
>> There shouldn't be any forwarder blocks in cfg layout mode. What did
>> you need this for?
>
> This was a performance fix.
>
> There is code in try_forward_edges, called from try_optimize_cfg that
> we call from cleanup_cfg, typically in cfglayout mode, that will not
> eliminate forwarding blocks when either the given block "b" or its
> successor block ends with a region-crossing jump. The comments
> indicate that these need to be left in to ensure we don't fall through
> across section boundaries, which makes sense. The issue here was that
> I saw the blocks in the hot partition ending in conditional branches,
> which had a fall-through to another hot section block, and the
> conditional jump led to yet another block in the hot section that
> simply contained an unconditional jump to a cold section block. So in
> this case when try_forward_edges was called with the block with the
> conditional branch, when we look at its successor (the forwarding
> block), we can't eliminate it since it ends in a region crossing
> branch. I guess the concern is that if the conditional branch sense
> was reversed in cfglayout mode we would end up falling through to a
> different region. But once we leave cfglayout mode that should not
> occur. So I loosened up the checks on the successor block so that it
> is ok if it ends in a region crossing branch when we are in cfgrtl
> mode (and added this call). That way, these forwarding blocks are
> eliminated and we are able to have a region crossing conditional jump
> directly to the cold section block, without the intervening forwarding
> block.

This sounds a bit scary to me. After bbro there shouldn't be any
changes to the basic block layout anymore. Do you have a test case for
this problem? I'd like to understand why those forwarder blocks are
really necessary. In cfglayout mode, it's supposed to be simpler to
modify the CFG without the constraints of cfgrtl mode. You describe a
scenario where cfglayout mode is more constrained than cfgrtl, and
that'd be a bug IMHO.

Ciao!
Steven