Re: Fix PR 53743 and other -freorder-blocks-and-partition failures

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

On Wed, May 22, 2013 at 7:17 AM, Teresa Johnson wrote:
> On Mon, May 20, 2013 at 3:42 PM, Steven Bosscher wrote:

>> But to be honest, I still don't really understand why we emit a
>> barrier at all if we're in cfglayout mode. They're ignored, they're
>> going to be overlooked if someone looks for barriers after basic
>> blocks (nobody looks in the footer, and they should not), and
>> fixup_reorder_chain ought to put barriers where needed when coming out
>> of cfglayout mode. (TODO++ - that's how it goes for me every time I
>> look at gcc code ;-)
>
> Ok, I've moved it. Interestingly, previous to my modification to
> enable this to be called in non-cfglayout mode it was only called from
> bbpart, which is in cfglayout mode. This may be another case where
> bbpart is confused about what mode it is in? Probably worth some
> investigation/cleanup at some point as you note.

The problem here is two things:

1. Many GCC developers still don't fully grasp the difference between
cfglayout mode and the older cfgrtl mode.

2. There isn't anything in rtl_verify_flow_info that verifies
cfglayout assumptions (such as "nothing lives between basic blocks")

Cleaning that up is just yet another thing on my TODO list. If you
have the opportunity to spend some time on it (maybe starting with
some verify_flow_info bits), that'd be really welcome.



>>>         {
>>> -         emit_note_before (NOTE_INSN_SWITCH_TEXT_SECTIONS, BB_HEAD (bb));
>>> -         break;
>>> +         if (switched_sections)
>>> +           {
>>> +             error ("multiple hot/cold transitions found (bb %i)",
>>> +                    bb->index);
>>> +             err = 1;
>>
>> Just gcc_assert (! switched_sections).
>
> The rest of the rtl flow verification code used error () and returned
> an err=1, so I was trying to be consistent with that. Do we want to be
> different in this routine?

But this is in insert_section_boundary_note, which isn't really
verification code. I haven't looked - does rtl_verify_flow_info
already check that there is only one section switch? If not, and this
code in insert_section_boundary_note is actually verification code,
then this code should probably be moved to cfgrtl.c to be called by
rtl_verify_flow_info. Can be done as a follow-up, though.



> I manually ran all of the gcc.c-torture/execute tests to collect and
> feedback fdo, and enable -freorder-blocks-and-partitions, with and
> without my fixes. This gave me a few test cases that I will add as
> fdo/partitioning test cases.

Impressive!

> A couple were still problems that I
> hadn't flushed out yet, relating to the new verification code that
> checks that we only have a single section switch in the function,
> because a couple post-bbro phases (e.g. compgoto) were not being
> careful enough.

Uh-oh, that compgoto pass is my doing. If you file a PR for it, please
assign it to me :-)

Ciao!
Steven