Re: [patch] Preserve the CFG until after final

[Steven Bosscher <stevenb dot gcc at gmail dot com>]

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