Message ID | 52CC265D.3000104@codesourcery.com |
---|---|
State | New |
Headers | show |
Hi Bernd, The patch is OK to me. But do we need reorder_loops for the c6x backend ? I mean we can set the do_reorder parameter to FALSE to save compile time, since c6x backend only choose hw-doloops whose body contains only one basic block. Cheers, Felix > > On 01/05/2014 05:10 PM, Teresa Johnson wrote: > > On Sun, Jan 5, 2014 at 3:39 AM, Bernd Schmidt <bernds@codesourcery.com> > wrote: > >> I have a different patch which I'll submit next week after some more > >> testing. The assert in cfgrtl is unnecessarily broad and really only > >> needs to trigger if -freorder-blocks-and-partition; there's nothing > >> wrong with entering cfglayout after normal bb-reorder. > > > > Currently -freorder-blocks-and-partition is the default for x86. I > > assume that hw-doloop is not enabled for any i386 targets, which is > > why we haven't seen this? > > Precisely. > > > And will this mean that -freorder-blocks-and-partition cannot be used > > for the targets that use hw-doloop? If so, should > > -freorder-blocks-and-partition be prevented with a warning for those > > targets? > > If someone explicitly chooses that option we can turn off the reordering in > hw-doloop. That should happen sufficiently rarely that it isn't a problem. That's > what the patch below does - bootstraped on x86_64-linux, tested there and > with bfin-elf. Ok? > > >> I've also tested that Blackfin still benefits from the hw-doloop > >> reordering code and generates more hardware loops if it's enabled. So > >> we want to be able to run it at -O2. > > > > I looked at hw-doloop briefly and since it seems to be doing some > > manual bb reordering I guess it can't simply be moved before bbro. It > > seems like a better long-term solution would be to make bbro > > hw-doloop-aware as Felix suggested earlier. > > Maybe. It could be argued that the code in hw-doloop is relevant only for a > small class of targets so it should only be enabled for them. In any case, that's > not stage 3 material and two ports are broken... > > > Bernd
On Tue, Jan 7, 2014 at 8:07 AM, Bernd Schmidt <bernds@codesourcery.com> wrote: > On 01/05/2014 05:10 PM, Teresa Johnson wrote: >> >> On Sun, Jan 5, 2014 at 3:39 AM, Bernd Schmidt <bernds@codesourcery.com> >> wrote: >>> >>> I have a different patch which I'll submit next week after some more >>> testing. The assert in cfgrtl is unnecessarily broad and really only >>> needs >>> to trigger if -freorder-blocks-and-partition; there's nothing wrong with >>> entering cfglayout after normal bb-reorder. >> >> >> Currently -freorder-blocks-and-partition is the default for x86. I >> assume that hw-doloop is not enabled for any i386 targets, which is >> why we haven't seen this? > > > Precisely. > > >> And will this mean that -freorder-blocks-and-partition cannot be used >> for the targets that use hw-doloop? If so, should >> -freorder-blocks-and-partition be prevented with a warning for those >> targets? > > > If someone explicitly chooses that option we can turn off the reordering in > hw-doloop. That should happen sufficiently rarely that it isn't a problem. > That's what the patch below does - bootstraped on x86_64-linux, tested there > and with bfin-elf. Ok? Ok, looks good to me. > > >>> I've also tested that Blackfin still benefits from the hw-doloop >>> reordering >>> code and generates more hardware loops if it's enabled. So we want to be >>> able to run it at -O2. >> >> >> I looked at hw-doloop briefly and since it seems to be doing some >> manual bb reordering I guess it can't simply be moved before bbro. It >> seems like a better long-term solution would be to make bbro >> hw-doloop-aware as Felix suggested earlier. > > > Maybe. It could be argued that the code in hw-doloop is relevant only for a > small class of targets so it should only be enabled for them. In any case, > that's not stage 3 material and two ports are broken... Ok, that makes sense. Thanks, Teresa > > > Bernd >
On 01/07/14 09:07, Bernd Schmidt wrote: > On 01/05/2014 05:10 PM, Teresa Johnson wrote: >> On Sun, Jan 5, 2014 at 3:39 AM, Bernd Schmidt >> <bernds@codesourcery.com> wrote: >>> I have a different patch which I'll submit next week after some more >>> testing. The assert in cfgrtl is unnecessarily broad and really only >>> needs >>> to trigger if -freorder-blocks-and-partition; there's nothing wrong with >>> entering cfglayout after normal bb-reorder. >> >> Currently -freorder-blocks-and-partition is the default for x86. I >> assume that hw-doloop is not enabled for any i386 targets, which is >> why we haven't seen this? > > Precisely. > >> And will this mean that -freorder-blocks-and-partition cannot be used >> for the targets that use hw-doloop? If so, should >> -freorder-blocks-and-partition be prevented with a warning for those >> targets? > > If someone explicitly chooses that option we can turn off the reordering > in hw-doloop. That should happen sufficiently rarely that it isn't a > problem. That's what the patch below does - bootstraped on x86_64-linux, > tested there and with bfin-elf. Ok? Yes. This is fine. jeff
On 01/09/2014 05:20 AM, Jeff Law wrote: > On 01/07/14 09:07, Bernd Schmidt wrote: >> If someone explicitly chooses that option we can turn off the reordering >> in hw-doloop. That should happen sufficiently rarely that it isn't a >> problem. That's what the patch below does - bootstraped on x86_64-linux, >> tested there and with bfin-elf. Ok? > Yes. This is fine. Looks like I got sufficiently distracted by travel and other things that this never got checked in. Now fixed. Bernd
* cfgrtl.c (cfg_layout_initialize): Weaken assert to only trigger if flag_reorder_blocks_and_partition. * hw-doloop.c (reorg_loops): Avoid reordering if that flag is set. diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c index 1a63249..2d75845 100644 --- a/gcc/cfgrtl.c +++ b/gcc/cfgrtl.c @@ -4222,14 +4222,14 @@ cfg_layout_initialize (unsigned int flags) rtx x; basic_block bb; - /* Once bb reordering is complete, cfg layout mode should not be re-entered. - Entering cfg layout mode will perform optimizations on the cfg that - could affect the bb layout negatively or even require fixups. An - example of the latter is if edge forwarding performed when optimizing - the cfg layout required moving a block from the hot to the cold section - under -freorder-blocks-and-partition. This would create an illegal - partitioning unless some manual fixup was performed. */ - gcc_assert (!crtl->bb_reorder_complete); + /* Once bb partitioning is complete, cfg layout mode should not be + re-entered. Entering cfg layout mode may require fixups. As an + example, if edge forwarding performed when optimizing the cfg + layout required moving a block from the hot to the cold + section. This would create an illegal partitioning unless some + manual fixup was performed. */ + gcc_assert (!(crtl->bb_reorder_complete + && flag_reorder_blocks_and_partition)); initialize_original_copy_tables (); diff --git a/gcc/hw-doloop.c b/gcc/hw-doloop.c index b6184a2..9c2c874 100644 --- a/gcc/hw-doloop.c +++ b/gcc/hw-doloop.c @@ -636,7 +636,9 @@ reorg_loops (bool do_reorder, struct hw_doloop_hooks *hooks) loops = discover_loops (&loop_stack, hooks); - if (do_reorder) + /* We can't enter cfglayout mode anymore if basic block partitioning + already happened. */ + if (do_reorder && !flag_reorder_blocks_and_partition) { reorder_loops (loops); free_loops (loops);