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

[Jeff Law <law at redhat dot com>]

On 11/18/13 12:33, Teresa Johnson wrote:
> On Mon, Nov 18, 2013 at 7:34 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> > On Mon, Nov 18, 2013 at 3:53 PM, Teresa Johnson wrote:
> > >  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?
> >
> > It was more a hack that got out of hand. Apparently it hurt some
> > interpreters (branch prediction!) when the unified computed goto is
> > not "unfactored". There was a PR for this, and the unfactoring code I
> > added only fixed part of the problem.
> >
> >
> > > 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.
> >
> > Theoretically: Yes, perhaps. In practice there isn't much to gain.
> > Unfactoring before bb-reorder is probably the most helpful thing, to
> > get better results for basic block alignment and placement. But
> > scheduling punts on computed gotos (or explodes in some non-linear
> > bottleneck).
> >
> > What used to help is profile-based branch speculation, i.e.
> >
> > if (*target_addr == most_frequent_target_addr)
> >    goto most_frequent_target_add;
> > else
> >    goto *target_addr;
> >
> > But I'm not sure if our value profiling based optimizations still have
> > this case.
> >
> >
> > > 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.
> >
> > ISTR it's http://gcc.gnu.org/PR26854
>
> Ok, I have confirmed that moving compgotos earlier, to just before bb
> reordering, passes an LTO profiledbootstrap with
> -freorder-blocks-and-partition forced on (with my prior patch to fixup
> the layout removed). I also added an assert to ensure we aren't going
> into cfglayout mode after bb reordering is complete. Currently I am
> running a normal bootstrap and regression test without
> -freorder-blocks-and-partition force enabled.
>
> I confirmed that we still apply compgotos and successfully unfactor
> the computed goto in the testcase from PR15242.
>
> I also tried the two test cases in PR26854 (all.i and compiler.i) both
> with -O1 -fschedule-insns2 (which shouldn't actually be affected since
> compgotos/bbro not on) and with -O3 -fschedule-insns, as described in
> the bug report. I compared the time and mem reports before and after
> my change to the compgotos pass placement and there is no significant
> difference in the time or memory used from bb reordering or other
> downstream passes.
>
> Here is the patch I am testing with a normal bootstrap and regression
> test on x86-64-unknown-linux-gnu. Is this change ok for trunk assuming
> that passes?
>
> Thanks,
> Teresa
>
> 2013-11-18  Teresa Johnson  <tejohnson@google.com>
>
>          * gcc/cfgrtl.c (cfg_layout_initialize): Assert if we
>          try to go into cfglayout after bb reordering.
>          * gcc/passes.def: Move compgotos before bb reordering
>          since it goes into cfglayout.
This is fine once the bootstrap/regression testing passes.

Thanks,
jeff