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