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

[Xinliang David Li <davidxl at google dot com>]

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.
>
> -  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...
> +  if (current_ir_type () == IR_RTL_CFGLAYOUT)
>      emit_barrier_after (BB_END (jump_block));
>
>
>>> Index: cfgrtl.c
>>> ===================================================================
>>> --- cfgrtl.c    (revision 198686)
>>> +++ cfgrtl.c    (working copy)
>>> @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3.  If not see
>>>  #include "tree.h"
>>>  #include "hard-reg-set.h"
>>>  #include "basic-block.h"
>>> +#include "bb-reorder.h"
>>
>>
>> You may need to modify Makefile.in to declare this new dependency.
>
> Eh, no. cfgrtl should not depend on bb-reorder.
>
> And cfglayout should not depend on basic block partitioning, either,
> so this change:
>
>>  /* Finalize the changes: reorder insn list according to the sequence specified
>> -   by aux pointers, enter compensation code, rebuild scope forest.  */
>> +   by aux pointers, enter compensation code, rebuild scope forest. If
>> +   this is called when we will FINALIZE_REORDER_BLOCKS, indicate that
>> +   to fixup_reorder_chain so that it can insert the proper switch text
>> +   section notes.  */
>>
>>  void
>> -cfg_layout_finalize (void)
>> +cfg_layout_finalize (bool finalize_reorder_blocks)
>
> is Just Wrong (tm).
>
> 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.

thanks,

David




>
> Ciao!
> Steven