On Mon, Aug 5, 2013 at 7:11 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
> The patch looks OK to me in general (I can not approve it).
> Still have one question...
>> +
>> +/* Ensure that no cold bbs dominate hot bbs along the dominance or
>> + post-dominance DIR, for example as a result of edge weight insanities.
>> + Returns the updated value of COLD_BB_COUNT and adds newly-hot bbs
>> + to BBS_IN_HOT_PARTITION. */
>> +
>> +static unsigned int
>> +sanitize_dominator_hotness (enum cdi_direction dir, unsigned int
>> cold_bb_count,
>> + vec<basic_block> *bbs_in_hot_partition)
>> +{
>> + /* Callers check this. */
>> + gcc_checking_assert (cold_bb_count);
>> +
>> + bool dom_calculated_here = !dom_info_available_p (dir);
>> +
>> + if (dom_calculated_here)
>> + calculate_dominance_info (dir);
>> +
>> + /* Keep examining hot bbs while we still have some left to check
>> + and there are remaining cold bbs. */
>> + vec<basic_block> hot_bbs_to_check = bbs_in_hot_partition->copy ();
>> + while (! hot_bbs_to_check.is_empty ()
>> + && cold_bb_count)
>> + {
>> + basic_block bb = hot_bbs_to_check.pop ();
>> + basic_block dom_bb = get_immediate_dominator (dir, bb);
>> +
>> + /* If bb's immediate dominator is also hot (or unpartitioned,
>> + e.g. the entry block) then it is ok. If it is cold, it
>> + needs to be adjusted. */
>> + if (BB_PARTITION (dom_bb) != BB_COLD_PARTITION)
>> + continue;
>
> What will hapepn on
>
> if (t)
> something
> else
> something else
> for (i=0;i<1000000;i++)
> something else2
>
> I would expect if/something and something else to be cold by profile feedback.
> Your dominator code will bring the if into hot partition but both paths
> of conditional will be cold, so the number of crossings will actually grow.
You are right, this case will not be handled well.
>
> If we want to have at least some path to hot blocks in the hot region, I
> suspect
> we could walk back from hot regions to entry and keep those in hot regions
> rather
> than relying on the dominator tree...
> But I am sure such things can be dealt with incrementally.
I am going to fix this and will resend the patch. Rather than look at
the immediate dominator of each hot block, we need to ensure that at
least one pred bb is hot. In your example, if that was a 50-50 branch,
then IMO both preds should be marked hot.
Thanks,
Teresa
>
> Honza
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
