On Fri, May 10, 2013 at 5:54 PM, Teresa Johnson wrote: > The main issue I had here, and why I made this change, is that we go > in and out of cfglayout mode several times after bb partitioning and > then out_of_cfglayout. The problem was that when we subsequently went > in and out of cfglayout mode, the switch text section notes that had > been inserted by bbpart were getting messed up (they were moved into > the bb header when we enter cfglayout mode and then not being > transferred to the correct location upon exit). > > I investigated trying > to keep those in sync, but it is really difficult/impossible to do > during cfglayout mode when they are in the header. So I simply strip > them out completely on entry to cfglayout mode, and if there were any > there on entry, this change ensures that they are restored in the > appropriate location upon exit. I'm not sure what is a good > alternative?
The problem is that the note exists at all. I'd like to see the note go away completely eventually (when we have a CFG all the way through pass_final). As a stop-gap, we should not emit the note until pass_free_cfg. Up to that point the basic blocks tell what partition they're in, and all hot block and cold blocks should be in a sequence. So during pass_free_cfg, just walk the basic blocks chain until there's a partition change, and emit the note between those two blocks. > I triggered the same error in 445.gobmk once I applied the > thread_prologue_and_epilogue_insns fixes. This is an assert in the > dwarf CFI code that complains about a NOTE_INSN_SWITCH_TEXTS_SECTION > note not being preceeded by a barrier: The problem here is that fixup_reorder_chain should force a jump for basic blocks in one partition falling through to bb->next. That should be dealt with in can_fallthru, which should return false if BB_PARTITION (target) != BB_PARTITION (src). > The correct solution in my opinion is to strip out the SWITCH note > every time we enter cfglayout mode after bbro, and then invoke > insert_section_boundary_note when leaving cfglayout (if one was found > on entry to that cfglayout mode) to reapply it. Please make the note go away completely before pass_free_cfg, and earn greater admiration than Zeus. The note always was wrong, and now you've shown it's also a problem. >> * 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. > > Looking back through the earlier email exchanges from last fall I > found some discussion on this where I had found a couple places > causing this to happen. Two were in > thread_prologue_and_epilogue_insns: when we duplicated tail blocks or > created a new block to hold a simple return, and redirected some of > the edges to the new copy. In some cases this caused edges that were > previously region crossing to become non-region crossing, and in other > cases the reverse happened. I think there are a couple of other cases > mentioned in the emails too, but I would need to do some more digging > to find them all. > > There were a few places in the code that tried to detect this type of > issue, and fix them up, but they weren't consistent and there were > other places that had the same issue. I've centralized all that > handling in this patch (fixup_partitions and fixup_partition_crossing, > called from a few places where we redirect edges) so that it is more > consistent and comprehensive. Right, I think it's good that you've centralized this code. But it seems to me that we should make sure that the hot blocks and cold blocks are still grouped (i.e. there is only one basic block B such that BB_PARTITION(B) != BB_PARTITION(B->next_bb). That is something your code doesn't handle, AFAICT. It's just one thing that's difficult to maintain, probably there are others. It's also something the partitioning verifier should check, i.e. that the basic blocks in hot and cold partitions are properly grouped. BTW1: I don't understand this comment: > + /* Invoke the cleanup again once we are out of cfg layout mode > + after committing the final bb layout above. This enables removal > + of forwarding blocks across the hot/cold section boundary when > + splitting is enabled that were necessary while in cfg layout > + mode. */ > + if (crtl->has_bb_partition) > + cleanup_cfg (CLEANUP_EXPENSIVE); There shouldn't be any forwarder blocks in cfg layout mode. What did you need this for? BTW2: We badly need to figure out a way to create test cases for FDO... :-( Ciao! Steven
