Fix PR 53743 and other -freorder-blocks-and-partition failures (issue6823047)

[Teresa Johnson]

On Thu, May 9, 2013 at 3:40 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Thu, May 9, 2013 at 11:42 PM, Diego Novillo wrote:
>> On 2013-05-08 01:13 , Teresa Johnson wrote:
>>> -static void
>>> +void
>>>  emit_barrier_after_bb (basic_block bb)
>>>  {
>>>    rtx barrier = emit_barrier_after (BB_END (bb));
>>> -  BB_FOOTER (bb) = unlink_insn_chain (barrier, barrier);
>>> +  if (current_ir_type () == IR_RTL_CFGLAYOUT)
>>> +    BB_FOOTER (bb) = unlink_insn_chain (barrier, barrier);
>>
>>
>> What if the current IR is not RTL?  Should we fail here?  It doesn't seem
>> like it makes sense to call this from gimple, for instance.
>
> It also makes no sense calling it in IR_RTL_CFGLAYOUT mode. Barriers
> are meaningless in cfglayout mode.

Actually, the change above ensures we can call this routine when *not*
in CFGLAYOUT mode. It was previously only called when we were in
CFGLAYOUT mode (while in bbpart).

(This relates to a conversation we had on an earlier version of the
patch back in the fall, where we discussed this issue of bbpart adding
barriers while in cfglayout mode:
http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02996.html).

>
> -  emit_barrier_after (BB_END (jump_block));
> +  /* We might be in cfg layout mode, and if so, the following routine will
> +     insert the barrier correctly.  */
> +  emit_barrier_after_bb (jump_block);
>
> We're practically always in cfglayout mode, but oh well...

Right, not always, which is why I needed to make this change.


On Fri, May 10, 2013 at 5:05 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Fri, May 10, 2013 at 12:57 AM, Xinliang David Li wrote:
>> On Thu, May 9, 2013 at 3:40 PM, Steven Bosscher wrote:
>
>>> This patch mixes up things badly from the point of
>>> what-depends-on-what, the whole approach looks wrong to me.
>>
>>
>> Do you mean the 'source file dependency' or 'logical dependency'?
>>
>> If the former, the code can be easily refactored to remove the
>> dependency. I don't see how the latter can be avoided as bb-partition
>> etc does change cfg states and leads to different actions in cfg
>> layout finalize.
>
> I mean logical dependency. cfglayout is just a representation of the
> CFG, and bb-partition is a code transformation. By making
> fixup_reorder_chain emit the note, you' ve put part of the
> transformation into out-of-cfglayout which is just bogus. You also
> don't put GCSE or loop unrolling in out-of-cfglayout, and this change
> is IMHO in the same category: mixing transformations into internal
> representations. That may be a short-term fix but it is a long-term
> maintenance/cleanups nightmare.

The main issue I had here, and why I made this change, is that we go
in and out of cfglayout mode several times after bb partitioning and
then out_of_cfglayout. The problem was that when we subsequently went
in and out of cfglayout mode, the switch text section notes that had
been inserted by bbpart were getting messed up (they were moved into
the bb header when we enter cfglayout mode and then not being
transferred to the correct location upon exit). I investigated trying
to keep those in sync, but it is really difficult/impossible to do
during cfglayout mode when they are in the header. So I simply strip
them out completely on entry to cfglayout mode, and if there were any
there on entry, this change ensures that they are restored in the
appropriate location upon exit. I'm not sure what is a good
alternative?

I found a more detailed description of why I made this change in our
email exchange from the fall:

--------------
I triggered the same error in 445.gobmk once I applied the
thread_prologue_and_epilogue_insns fixes. This is an assert in the
dwarf CFI code that complains about a NOTE_INSN_SWITCH_TEXTS_SECTION
note not being preceeded by a barrier:

gcc -c -o engine/utils.o -DSPEC_CPU -DNDEBUG -DHAVE_CONFIG_H -I. -I..
-I../include -I./include   -fprofile-use
-freorder-blocks-and-partition -freorder-blocks -ffunction-sections
 -O2      engine/utils.c

engine/utils.c: In function ‘visible_along_edge’:
engine/utils.c:274:1: internal compiler error: in create_pseudo_cfg,
at dwarf2cfi.c:2742
 }
 ^

In this case the switch section note was inside a BB. What I found was
that this was due to several phases going into and back out of
cfglayout mode again. In this case it was the compgotos phase. There
aren't any computed gotos, but this change occurs during
cfg_layout_initialize (in try_optimize_cfg called via cleanup_cfg).
Here it merged two (non-contiguous) blocks that had a
single-successor/single-predecessor relationship. However, the source
block was previously on the section boundary and had a SWITCH note
prior. This note is put into the header of the bb when we go into
cfglayout mode, and ended up inside the new merged block, which was in
any case not on the new border between the hot and cold sections.

The correct solution in my opinion is to strip out the SWITCH note
every time we enter cfglayout mode after bbro, and then invoke
insert_section_boundary_note when leaving cfglayout (if one was found
on entry to that cfglayout mode) to reapply it.
--------------

>
> Although I've only skimmed the patch, I have noted several issues with it:
>
> * 3 different changes put into a single patch: the
> crtl->has_bb_partition change (which looks good to me), the
> verification stuff, and various fixes. The patch should be submitted
> in 3 parts to make/testing review easier.

I plan to do that based on your, Jeff and Honza's suggestion.

>
> * Emitting barriers in cfglayout mode. That's non-sense.

See earlier description - that was always the case, not due to my change.

>
> * Fixup redirected edges that did not cross partitions before but
> apparently do after redirection. This is not supposed to happen in the
> first place, so fixing up any of this just papers over an error
> elsewhere in the compiler.

Looking back through the earlier email exchanges from last fall I
found some discussion on this where I had found a couple places
causing this to happen. Two were in
thread_prologue_and_epilogue_insns: when we duplicated tail blocks or
created a new block to hold a simple return, and redirected some of
the edges to the new copy. In some cases this caused edges that were
previously region crossing to become non-region crossing, and in other
cases the reverse happened. I think there are a couple of other cases
mentioned in the emails too, but I would need to do some more digging
to find them all.

There were a few places in the code that tried to detect this type of
issue, and fix them up, but they weren't consistent and there were
other places that had the same issue. I've centralized all that
handling in this patch (fixup_partitions and fixup_partition_crossing,
called from a few places where we redirect edges) so that it is more
consistent and comprehensive.

>
> * The fixup_reorder_chain changes I've mentioned above.

See explanation above.

Thanks!
Teresa

>
>
> Ciao!
> Steven



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