On Wed, May 22, 2013 at 10:21 AM, Eric Botcazou wrote: >> That is only partially true. Currently the transition is already de >> facto going on: Just look at how many back ends use >> compute_bb_for_insn to re-initialize the BLOCK_FOR_INSN pointers right >> after pass_free_cfg (it's usually the first thing they do in the >> machine-reorg pass). > > Yes, and we should do something about it. Btw, why is the line removed from > ia64_reorg in the patch (and not mentioned in the ChangeLog)?
Mistake. I wrote these patches on an ia64 machine, an older version of the patch was also bootstrapped&tested there. >> Some ports never call free_bb_or_insn after that, >> and expect BLOCK_FOR_INSN to be valid in 'final'. One of those ports >> is i386, look at where BLOCK_FOR_INSN is used in i386.c (in functions >> deciding what asm output templates to use). > > AFAICS it's only used for splitters. And using BLOCK_FOR_INSN after the last > split pass (pass_split_for_shorten_branches) is dubious. Actually it's even wrong *during* pass_split_for_shorten_branches right now. It may work in some ports but not in others, depending whether the port's machine reorg has done its TLC on the CFG or not. This can lead to very interesting behavior. I chased a bug recently where a splitter during split5 used the DF_INSN_USES cache on an insn introduced by another splitter. Because split5 uses split_all_insns_noflow, the insn from the splitter somehow ended up a NULL BLOCK_FOR_INSN (I'm still not sure how that happened, but it did). It turns out df-scan ignores insns with a NULL BLOCK_FOR_INSN, and hence NULL DF_INSN_USES, and that lead to a dependency violation in my splitter because a USE got overlooked! Ah, the crazy stuff one can do after machine reorg. It's the Wild West of GCC :-) > Here's the list: > > ./frv/frv.c: || BLOCK_FOR_INSN (insn) == ce_info->else_bb > > Only used during if-conversion. > > ./rs6000/rs6000.c: bb = BLOCK_FOR_INSN (label); > > Only used during compute_alignments. > > ./spu/spu.c: bitmap_set_bit (blocks, BLOCK_FOR_INSN (branch)->index); > > Only used during md_reorg. > > ./c6x/c6x.c: BLOCK_FOR_INSN (bundle) = BLOCK_FOR_INSN (slot[0]); > > Only used during md_reorg. > > ./mips/mips.c: basic_block bb = BLOCK_FOR_INSN (insn); > ./mips/mips.c: /* Restore the BLOCK_FOR_INSN pointers, which are needed by > DF. Also during > > Only used for splitting decision, deal with null BLOCK_FOR_INSN. Yes, these are all fine. > ./i386/i386.c: basic_block bb = start ? BLOCK_FOR_INSN (start) : NULL; > ./i386/i386.c: basic_block bb = BLOCK_FOR_INSN (insn); > ./i386/i386.c: basic_block bb = start ? BLOCK_FOR_INSN (start) : NULL; > ./i386/i386.c: basic_block bb = BLOCK_FOR_INSN (insn); > ./i386/i386.c: basic_block bb = BLOCK_FOR_INSN (insn); > ./i386/i386.c: rtx start = BB_HEAD (BLOCK_FOR_INSN (insn)); > ./i386/i386.c: basic_block bb = BLOCK_FOR_INSN (insn); > > Only used for splitting decision (and scheduling). Sadly no. Most of these (the *agu* ones) are also reached from final. For example: movdi_internal -> ix86_use_lea_for_mov -> ix86_lea_outperforms -> distance_non_agu_define -> distance_non_agu_define_in_bb Likewise for movsi_internal, and zero_extendsidi2. For the mov?i_internal define_insns, it's been like that since at least r181077 (November 2011). I must admit I was surprised by that, too. It may have been coincidence that it worked when this patch was (IMHO wrongfully) accepted. Someone got away with it because i386 calls compute_bb_for_insn in its machine-reorg, and does *not* call free_bb_for_insn, leaving the BLOCK_FOR_INSN pointers in place all the way through final. There are no passes between machine-reorg and final that run for i386 and damage the CFG because split5 doesn't run on i386 (because of STACK_REGS) and the other passes, like shorten_branches, don't modify the insns chain. Most ports that call compute_bb_for_insn do not call free_bb_for_insn: $ grep compute_bb_for_insn config/*/*.c config/arm/arm.c: compute_bb_for_insn (); config/bfin/bfin.c: compute_bb_for_insn (); config/c6x/c6x.c: compute_bb_for_insn (); config/frv/frv.c: compute_bb_for_insn (); config/i386/i386.c: compute_bb_for_insn (); config/ia64/ia64.c: compute_bb_for_insn (); config/mep/mep.c: compute_bb_for_insn (); config/mips/mips.c: compute_bb_for_insn (); config/mn10300/mn10300.c: compute_bb_for_insn (); config/picochip/picochip.c: compute_bb_for_insn (); config/spu/spu.c: compute_bb_for_insn (); config/spu/spu.c: compute_bb_for_insn (); config/tilegx/tilegx.c: compute_bb_for_insn (); config/tilepro/tilepro.c: compute_bb_for_insn (); $ grep free_bb_for_insn config/*/*.c config/mips/mips.c: free_bb_for_insn (); config/spu/spu.c: free_bb_for_insn (); config/spu/spu.c: free_bb_for_insn (); Most of these, eh, let's call them "compute_bb_for_insn ports", they come out of the machine reorg pass with an insns chain that doesn't pass verify_flow_info. The most common problem is that there are objects in the insns chain that verify_flow_info doesn't understand: Things like SEQUENCEs for bundles (c6x, mep), const-pool fake insns (arm), and other creative target-specific uses of RTL. But for some ports the CFG is Just Fine after machine reorg, and only split5 mangles it before final >> Also, right now I'm stuck with a chicken-and-egg problem: dbr_schedule >> is not CFG-aware, but my still slowly progressing work on a >> replacement can't have a CFG because it has to run after machine-reorg >> passes, and therefore after pass_free_cfg. > > That's why I'm suggesting to get rid of or modify pass_free_cfg so that the > CFG is still usable up to the point where it is really destroyed. I think you're taking a too dbr_schedule-ports point of view on this. There are already targets that never really destroy the CFG at all, all the way through final. Few ports that do destroy it, destroy it as badly as dbr_schedule. Most only have innocent "damage" that are really just deficiencies of verify_flow_info. >> > I think that an incremental step would be to allow the machine reorg >> > pass to use the CFG, even if it doesn't maintain it. >> >> That is the current state of things already. > > No, as you noted earlier, because of pass_free_cfg. Yes, because most compute_bb_for_insn-ports don't clear BLOCK_FOR_INSN anymore. Remember: the only thing that pass_free_cfg does, is clear the BLOCK_FOR_INSN pointers (OK, and as of very recently, it insert the hot/cold section boundary note), but edges and basic blocks are not released. Targets that compute_bb_for_insn and don't do any RTL tricks that confuse verify_flow_info all have a valid CFG. On i386, verify_flow_info still passes in 'final', even now! With my recog.c patch to use the CFG-aware split_all_insns if the CFG is still alive, verify_flow_info also passes in final for ia64. You are absolutely right that formally it should not work. But it does, given the right circumstances. All I want to do is make it formally OK for those targets that can handle the truth... >> But I need this exactly for that reason: To remove that blocker! :-) > > I don't think so. The goal for now shouldn't be to "preserve the CFG until > after final" since we know it isn't feasible in the short term, but rather to > preserve the CFG as long as possible. It is feasible in the short term -- as in: right now -- for some targets. Is it possible in the short term for all targets? No. But you've got to start somewhere. I firmly believe that port maintainers will not find it hard to make it work for their ports, dbr_schedule ports aside and that's a problem I'm trying to solve (while at it: ping**2 for http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00595.html that I need to move ahead). > So what about doing the following instead: > 1. Remove pass_free_cfg from the pipeline, > 2. Remove compute_bb_for_insn from all the md_reorg passes that call it, > 3. (Optionally) Do pass_free_cfg from all the md_reorg passes that don't, > 4. Do pass_free_cfg at the beginning of pass_delay_slots, > 5. Do pass_free_cfg at the end of pass_split_for_shorten_branches, I've considered that path, too, but I opted against it because we end up with pass_free_cfg being called from the majority of targets, and verify_flow_info calls in targets that really maintain a proper CFG. It also results in unnecessary damage from split5 for targets that have a valid CFG up to that point. But what worried me the most is that this approach made it more difficult to see what ports actually are CFG-safe. I chose the new target hook approach because you can just grep for the CFG-safe target hook to see what ports are already OK and which ones are still TODO. But if I still haven't convince you, I'll prepare a patch along those lines. Ciao! Steven
