On Sat, May 11, 2013 at 4:44 AM, Steven Bosscher <stevenb....@gmail.com> wrote:
> 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!
Some encouraging news: I removed the earlier calls to
insert_section_boundary_note and added a call from
rest_of_pass_free_cfg, and I also expanded that routine to do similar
sanity checking as in verify_hot_cold_block_grouping to ensure we only
switch sections once. And cpu2006 built fine with profile feedback and
splitting enabled. I am running them now, but all the previous
problems I chased down were compile-time not run-time, thankfully, so
that is a very good sign. I'll do some more testing with internal
benchmarks.
>
>>> 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.
Yes, although the new flag is not sufficient since it just indicates
that there was partitioning done and not that we are done with bb
reordering. I don't see a way to detect that we are done with that
pass, so it looks like I will need to add another flag to the rtl_data
struct.
>
> 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.
Yep, I agree it will be better to add the checking code to catch issues early.
>
>
>
>>>> + /* 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.
Let me find a small reproducer. I was looking at this on some internal
code a couple weeks ago. From my understanding of cfg layout, I
thought this was needed to enable the flexibility to redirect
branches, etc in that mode (i.e. enables the simplicity you mention
above). There is a comment block that shows up multiple places in the
cfg optimization-related files:
/* If we are partitioning hot/cold basic blocks, we don't want to
mess up unconditional or indirect jumps that cross between hot
and cold sections.
Basic block partitioning may result in some jumps that appear to
be optimizable (or blocks that appear to be mergeable), but which really
must be left untouched (they are required to make it safely across
partition boundaries). See the comments at the top of
bb-reorder.c:partition_hot_cold_basic_blocks for complete details. */
And at least a bunch of these are when we are in cfglayout mode.
But let me locate a reproducer so we can make sure it isn't due to
some issue with my patch.
Thanks,
Teresa
>
> Ciao!
> Steven
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
