On Mon, Nov 18, 2013 at 12:23 PM, Jeff Law <l...@redhat.com> wrote: > On 11/18/13 12:33, Teresa Johnson wrote: >> >> On Mon, Nov 18, 2013 at 7:34 AM, Steven Bosscher <stevenb....@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 <tejohn...@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.
The bootstrap/regression test passed. Added a comment as Honza suggested and committed as r204985. Thanks, Teresa > > Thanks, > jeff -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
