On Fri, Aug 2, 2013 at 8:05 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
>>
>> 2013-08-01 Teresa Johnson <tejohn...@google.com>
>> Steven Bosscher <ste...@gcc.gnu.org>
>>
>> * cfgrtl.c (fixup_bb_partition): New routine.
>> (commit_edge_insertions): Invoke fixup_partitions.
>> (find_partition_fixes): New routine.
>> (fixup_partitions): Ditto.
>> (verify_hot_cold_block_grouping): Update comments.
>> (rtl_verify_edges): Invoke find_partition_fixes.
>> (rtl_verify_bb_pointers): Update comments.
>> (rtl_verify_bb_layout): Ditto.
>> * basic-block.h (fixup_partitions): Declare.
>> * cfgcleanup.c (try_optimize_cfg): Invoke fixup_partitions.
>> * bb-reorder.c (sanitize_dominator_hotness): New function.
>> (find_rarely_executed_basic_blocks_and_crossing_edges): Invoke
>> sanitize_dominator_hotness.
>>
>> Index: cfgrtl.c
>> ===================================================================
>> --- cfgrtl.c (revision 201281)
>> +++ cfgrtl.c (working copy)
>> @@ -1341,6 +1341,34 @@ fixup_partition_crossing (edge e)
>> }
>> }
>>
>> +/* Called when block BB has been reassigned to a different partition,
>> + to ensure that the region crossing attributes are updated. */
>> +
>> +static void
>> +fixup_bb_partition (basic_block bb)
>> +{
>> + edge e;
>> + edge_iterator ei;
>> +
>> + /* Now need to make bb's pred edges non-region crossing. */
>> + FOR_EACH_EDGE (e, ei, bb->preds)
>> + {
>> + fixup_partition_crossing (e);
>> + }
>> +
>> + /* Possibly need to make bb's successor edges region crossing,
>> + or remove stale region crossing. */
>> + FOR_EACH_EDGE (e, ei, bb->succs)
>> + {
>> + if ((e->flags & EDGE_FALLTHRU)
>> + && BB_PARTITION (bb) != BB_PARTITION (e->dest)
>> + && e->dest != EXIT_BLOCK_PTR)
>> + force_nonfallthru (e);
>> + else
>> + fixup_partition_crossing (e);
>> + }
>> +}
>
> Is there particular reason why preds can not be fallhtrus and why
> force_nonfallthru edge does not need partition crossing fixup?
> (if so, perhpas it could be mentioned in the description, if not,
> I think force_nonfallthru path has to check if new BB was introduced
> and do the right thing on the edge.
I need to clarify the comments in this routine, because without the
context of how this is called it isn't clear. This routine is only
called when we detect a hot bb that is now dominated by a cold bb and
needs to become cold. Therefore, its preds will no longer be region
crossing (any non-dominating blocks that were previously hot would
have been marked cold in the caller for the same reason, so we will
not end up adjusting the region crossing-ness or fallthrough-ness of
those pred edges). Any that were region crossing before but aren't any
longer could not have been fall through (as Steven noted, you can't
have a fall through across a partition boundary). I will add some
better comments here.
Regarding the call to force_nonfallthru, that routine calls
fixup_partition_crossing as needed, and I will update the comment to
reflect that too.
>
>> +/* Sanity check partition hotness to ensure that basic blocks in
>> + the cold partition don't dominate basic blocks in the hot partition.
>> + If FLAG_ONLY is true, report violations as errors. Otherwise
>> + re-mark the dominated blocks as cold, since this is run after
>> + cfg optimizations that may make hot blocks previously reached
>> + by both hot and cold blocks now only reachable along cold paths. */
>
> With profile, I suppose we can have cold blocks dominating hot blocks when the
> hot blocks is in loop whose trip count is high enough. Indeed for
> partitioning
> reasons it does not make sense to push those into different section.
>
> I also wonder, if we finally get the pass stable, can we enable it by default
> and offline probably cold blocks w/o profile? Primarily blocks reachable only
> by EH + blocks leading to a crash or throw(). For C++ those should be common
> enough to make a difference...
Yep, as soon as PR57451 is fixed, which I hope to get to next week,
then I am going to send a patch to turn this on by default, at least
with profile feedback, which is where I've been doing performance
tuning. But you are right that there are some cases where it should be
beneficial without profile data as well.
Thanks,
Teresa
>
> Honza
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
