On Apr 10, 2007, at 2:44 PM, Steven Bosscher wrote:
> On Tuesday 10 April 2007 20:51, Robert Kidd wrote:
>> On Apr 9, 2007, at 11:07 AM, Steven Bosscher wrote:
>>> Can you please send an updated patch? The one from the link
>>> does not apply anymore to the current trunk.
>>
>> Certainly.
>
> Thank you.
> Looks all pretty good to me. I still have a few questions though:
>
>> @@ -276,26 +301,25 @@ tail_duplicate (void)
>> && can_duplicate_block_p (bb2))
>> {
>> edge e;
>> - basic_block old = bb2;
>> -
>> - e = find_edge (bb, bb2);
>> + basic_block copy;
>>
>> nduplicated += counts [bb2->index];
>> - bb2 = duplicate_block (bb2, e, bb);
>>
>> - /* Reconsider the original copy of block we've duplicated.
>> - Removing the most common predecessor may make it to be
>> - head. */
>> - blocks[old->index] =
>> - fibheap_insert (heap, -old->frequency, old);
>> + e = find_edge (bb, bb2);
>> +
>> + copy = duplicate_block (bb2, e, bb);
>> + flush_pending_stmts (e);
>> +
>> + add_phi_args_after_copy (©, 1);
>
> I don't understand why you've removed the code to reconsider the
> original copy. This is not in your ChangeLog entry, but I assume
> you have a reason why you remove this. Can you explain this please?
Oop, I should have caught that. The first version of this patch
contained a loop to repair PHI nodes near that point. I accidentally
removed that bit when I changed the code to call
add_phi_args_after_copy. I'll add this back in and retest.
>> -/* Connect the superblocks into linear sequence. At the moment
>> we attempt to keep
>> - the original order as much as possible, but the algorithm may
>> be made smarter
>> - later if needed. BB reordering pass should void most of the
>> benefits of such
>> - change though. */
>> -
>> -static void
>> -layout_superblocks (void)
>> -{
>
> *snif* goodbye...
>
> I suppose removing this is a good idea, but it makes me wonder if we
> should perhaps run tracer only if we know that the BB reordering pass
> will run. In other words:
>
>> +gate_tracer (void)
>> {
>> return (optimize > 0 && flag_tracer);
>> }
>
> Check "(optimize && flag_tracer && flag_reorder_blocks)". What do you
> think of this?
That would probably be a good idea. Superblock formation's control
flow specialization will probably help high level optimizations in
either case. However, if blocks are not reordered, I could believe
that the specialized code could hurt instruction cache locality. It
makes sense to do Superblock only when you know that you'll do both
high level opti and block reordering. I'll try this out.
Thanks
Robert