Re: [PATCH] Fix lto bootstrap verification failure with -freorder-blocks-and-partition

[Teresa Johnson <tejohnson at google dot com>]

On Sat, Nov 16, 2013 at 12:33 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> When testing with -freorder-blocks-and-partition enabled, I hit a
>> verification failure in an LTO profiledbootstrap. Edge forwarding
>> performed when we went into cfg layout mode after bb reordering
>> (during compgotos) created a situation where a hot block was then
>> dominated by a cold block and was therefore remarked as cold. Because
>> bb reorder was complete at that point, it was not moved in the
>> physical layout, and we incorrectly went in and out of the cold
>> section multiple times.
>>
>> The following patch addresses that by fixing the layout when we move
>> blocks to the cold section after bb reordering is complete.
>>
>> Tested with an LTO profiledbootstrap with
>> -freorder-blocks-and-partition enabled. Ok for trunk?
>>
>> Thanks,
>> Teresa
>>
>> 2013-11-15  Teresa Johnson  <tejohnson@google.com>
>>
>>         * cfgrtl.c (fixup_partitions): Reorder blocks if necessary.
>
> computed_gotos just unfactors unified blocks that we use to avoid CFGs with
> O(n^2) edges. This is mostly to avoid problems with nonlinearity of other passes
> and to reduce the quadratic memory use case to one function at a time.
>
> I wonder if it won't be cleaner to simply unfactor those just before pass_reorder_blocks.
>
> Computed gotos are used e.g. in libjava interpreter to optimize the tight interpretting
> loop.  I think those cases would benefit from having at least scheduling/reordering
> and alignments done right.
>
> Of course it depends on how bad the compile time implications are (I think in addition
> to libjava, there was a lucier's testcase that made us to go for this trick) ,
> but I would prefer it over ading yet another hack into cfgrtl...
> We also may just avoid cfglayout cleanup_cfg while doing computed gotos...

I am testing a new patch right now that simply moves compgotos to just
before bb reordering, and ads an assert to cfg_layout_initialize to
detect when we attempt that after bb reordering. I looked at the other
places that go into cfg layout and I think compgotos is currently the
only one after bb reordering.

>From a bb layout perspective it seems like it would be beneficial to
do compgotos before layout. Was the current position just to try to
reduce compile time by keeping the block unified as long as possible?
For your comment "I think those cases would benefit from having at
least scheduling/reordering and alignments done right." do you mean
that it would be better from a code quality perspective for those to
have compgotos done earlier before those passes? That seems to make
sense to me as well.

I'm doing an lto profiledbootstrap with the change right now. Is there
other testing that should be done for this change? Can you point me at
lucier's testcase that you refer to above? I found that PR15242 was
the bug that inspired adding compgotos, but it is a small test case so
I'm not sure what I will learn from trying that other than whether
compgotos still kicks in as expected.

Thanks,
Teresa

>
> Honza



-- 
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413