On Thu, May 9, 2013 at 3:40 PM, Steven Bosscher <stevenb....@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
