This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Do not set BB_SUPERBLOCK in loop-unroll.c:split_edge_and_insert
Hello,
> A desparate note
> ============
>
> I keep running into the problem that I just have no way to figure out
> where a fallthrough edge goes to in cfglayout mode. Why do we not want
> JUMP_INSNs again in cfglayout mode? It drives me crazy.
:-) (or :-( ?)
> Problems are the following:
>
> * find_bb_boundaries removes all outgoing edges, so here we lose track
> of where a fallthrough edge goes to in cfglayout mode. This is
> unrecoverable information in the current implementation. My
> work-around is to not remove the edges, but this assumes that those
> edges are OK and I don't think I can rely on that, generally.
I think you can, the only edges for that there may not be enough information
are the fallthru edges from its end, and they must still lead from the end of
the last of the blocks created by splitting.
> * In make_edges, cfgbuild.c:392-409, gcc wants to make an edge to
> bb->next_bb. I want this for sub basic blocks that
> find_sub_basic_blocks may have found, but I don't want this for the
> original blocks -- again, I assume the existing outgoing edges are
> valid. My work-around for now is to set BLOCK_ORIGINAL on the last
> block that find_bb_boundaries finds (which may be the original block
> if there are no sub basic blocks).
This sounds OK to me.
> * Also in make_edges, gcc will try to make an edge from
> ENTRY_BLOCK_PTR to ENTRY_BLOCK_PTR->next_bb, but there doesn't have to
> be an edge there in cfglayout mode.
Yes, you do not want to change the edge from ENTRY_BLOCK_PTR.
> All this together results in the attached patch, which does not work,
> unsurprisingly. I get functions "optimized" to empty functions,
> indicating there are missing edges. I have no clue what else to do at
> this point.
A testcase? I may have a look at what the problem is; so far, all you
said looks basically OK to me.
>
> A happy note
> ============
>
> I believe I've found some "proof" that the BB_SUPERBLOCK flag is
> unnecessary in split_edge_and_insert:
> 1. There is a verify_flow_info_call in
> loop-init.c:loop_optimizer_finalize(), which runs before
> break_superblocks.
> 2. In verify_flow_info, we do not handle BB_SUPERBLOCK, so a control
> flow insn inside a basic block would trigger an ICE even if the
> BB_SUPERBLOCK flag is set on that block.
> 3. We have no reported ICEs of this kind.
>
> Therefore, Zdenek, can you live with a patch that removes the line
> that sets BB_SUPERBLOCK in split_edge_and_insert, and adds a comment
> with your analysis of why it might be needed but we don't know for
> sure?
Yes, I am now persuaded that this might be the best option.
> A sad note
> ============
>
> Even with the BB_SUPERBLOCK flag out of the way, I now run into new
> problems, because of Ian's lower-subreg pass. Instead of fixing the
> CFG in place when necessary, Ian makes the pass use
> find_many_sub_basic_blocks to fix up some rare cases where additional
> control flow edges may be required after lowering subregs.
>
> And, you've guessed it, find_many_sub_basic_blocks doesn't work in
> cfglayout mode.
>
> So now I can stay in cfglayout mode over even fewer passes, until I
> (or, I'd really hope, Ian) will have taught lower-subreg to update the
> CFG in place. Ian has already told me why he needed this, and I hope
> we can find some time to solve the issue in a cleaner way.
Another possibility would be to finish the patch you posted (which I think
should not be all that hard, according to your description, I think
only some minor problem(s) are left).
> Can we please put a "ban" on new uses of find_many_sub_basic_blocks?
>
> This function should only be used by passes with no proper control
> over the transformations that they do.
>
> As semi-proof of that, consider
> the short list of passes, other than lower-subreg, that use it:
> cfgexpand, the insn splitting passes, rtl-eh, and reload, i.e. the
> usual list of passes that play "nice" on the CFG ;-)
>
> The reason why find_many_sub_basic_blocks should be avoided is because
> it basically destroys parts of the CFG in order to reconstruct it from
> scratch. It destroys profile/predictions information in the process,
> and that is unrecoverable. So this is IMHO a harmful Big Hammer for
> lazy passes that should just work a little harder to do their job
> right.</complaint> ;-)
Unfortunately, I think there might be cases where you simply do not have
choice -- for example, if your pass uses md expanders that just happen
to produce branching code (this definitely is the case for cfgexpand).
Regarding the profile, I have a patch
http://gcc.gnu.org/ml/gcc-patches/2006-07/msg00893.html
that somewhat helps (and I should get to updating and commiting it :-)
Zdenek