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

[Teresa Johnson <tejohnson at google dot com>]

On Wed, May 22, 2013 at 3:07 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> 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.

Yep, I'll try to do more cleanup. I'm still working on performance
tuning with splitting, so more cleanup may come out of that as well.

>
>
>
>>>>         {
>>>> -         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.

Oh got it, there is nearly identical code in
verify_hot_cold_block_grouping, and I got confused. I can fix this.
It's not fixed in the newest version of the patch I just sent out an
hour ago.

>
>
>
>> 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 :-)

It's fixed in the patch I just sent. See if things look ok there.

Thanks!
Teresa

>
> Ciao!
> Steven



--
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413