This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Fix PR 53743 and other -freorder-blocks-and-partition failures (issue6823047)
- From: Steven Bosscher <stevenb dot gcc at gmail dot com>
- To: Xinliang David Li <davidxl at google dot com>
- Cc: Diego Novillo <dnovillo at google dot com>, Teresa Johnson <tejohnson at google dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Fri, 10 May 2013 14:05:50 +0200
- Subject: Re: Fix PR 53743 and other -freorder-blocks-and-partition failures (issue6823047)
- References: <20130508050850 dot 923A7809EE at tjsboxrox dot mtv dot corp dot google dot com> <CAAe5K+UW5MfcbaHCp+zWTGM9fZvKm6x6no9WDrOOKsap0vvxUQ at mail dot gmail dot com> <518C1851 dot 7030903 at google dot com> <CABu31nNmHHvnatfVcRucsnv8yhQ_Ud-SkunECcc7pLq+L20_gg at mail dot gmail dot com> <CAAkRFZ+ezFmGhxpOg1Md=rFV1=+jUu83ut_O1mHvz2rrcTDSOQ at mail dot gmail dot com>
On Fri, May 10, 2013 at 12:57 AM, Xinliang David Li wrote:
> On Thu, May 9, 2013 at 3:40 PM, Steven Bosscher wrote:
>> This patch mixes up things badly from the point of
>> what-depends-on-what, the whole approach looks wrong to me.
>
>
> Do you mean the 'source file dependency' or 'logical dependency'?
>
> If the former, the code can be easily refactored to remove the
> dependency. I don't see how the latter can be avoided as bb-partition
> etc does change cfg states and leads to different actions in cfg
> layout finalize.
I mean logical dependency. cfglayout is just a representation of the
CFG, and bb-partition is a code transformation. By making
fixup_reorder_chain emit the note, you' ve put part of the
transformation into out-of-cfglayout which is just bogus. You also
don't put GCSE or loop unrolling in out-of-cfglayout, and this change
is IMHO in the same category: mixing transformations into internal
representations. That may be a short-term fix but it is a long-term
maintenance/cleanups nightmare.
Although I've only skimmed the patch, I have noted several issues with it:
* 3 different changes put into a single patch: the
crtl->has_bb_partition change (which looks good to me), the
verification stuff, and various fixes. The patch should be submitted
in 3 parts to make/testing review easier.
* Emitting barriers in cfglayout mode. That's non-sense.
* Fixup redirected edges that did not cross partitions before but
apparently do after redirection. This is not supposed to happen in the
first place, so fixing up any of this just papers over an error
elsewhere in the compiler.
* The fixup_reorder_chain changes I've mentioned above.
Ciao!
Steven