| 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);