Re: [patch] cfglayout fixes

[Jakub Jelinek <jakub at redhat dot com>]

On Wed, Dec 12, 2012 at 11:29:28AM +0100, Steven Bosscher wrote:
> >>         (fixup_reorder_chain): Set cfg_layout_function_header to NULL to
> >>         avoid dangling pointers into GC-allocated insns.  Clear BB_HEADER,
> >>         BB_FOOTER, and cfg_layout_function_footer for the same reason.
> >>         Do not link new barriers for cfgrtl mode to cfglayout's BB_FOOTER.
> >>         * combine.c (combine_instructions): Fix end-of-block check to not
> >>         expect BARRIERs, which may not exist in cfglayout mode.
> >
> > For the rest, I wonder if this really is stage3 material.  Do you have a
> > testcase that ICEs or is miscompiled without the changes?
> 
> The fixup_reorder_chain patch fixes and ICE with hot-cold partitioning
> and GCAC checking. A BARRIER is removed somewhere along the line, but
> the BB_FOOTER pointer still points to it. I'm not sure how things go
> bad from there, but we end up with BB_FOOTER pointing to a collected
> barrier.

Then a testcase should be added together with the patch and the change
shouldn't be part of further unrelated changes.

> The combine fix is a fix for a real bug. Your patch makes combine look
> across basic block boundaries and look for BARRIERs which can never be
> there. Technically this is a regression.

I don't see how.  If there is no barrier, but the prev note or insn doesn't
belong to the current bb, then BLOCK_FOR_INSN (last_combined_insn) != this_basic_block
will be true, and if there is no insn at all, last_combined_insn will be
NULL.  I view your combine.c change purely as a cleanup, thus IMHO stage 1
material.

	Jakub