Re: [PATCH] New switch optimization pass (PR tree-optimization/54742)

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, May 16, 2013 at 5:38 AM, Jeff Law <law@redhat.com> wrote:
> On 05/15/2013 12:28 PM, Steve Ellcey wrote:
>>
>> Here is a patch that adds a flag to gimple_duplicate_sese_region to tell
>> it whether or not to update the dominator information.  I had to add the
>> same flag to copy_bbs to make it all work.  How does this look?  I
>> tested it with a bootstrap and test on x86 (with my optimization
>> enabled) and got no regressions.
>>
>> 2013-05-15  Steve Ellcey  <sellcey@imgtec.com>
>>
>>         * cfghooks.c (copy_bbs): Add update_dominance argument.
>>         * cfghooks.h (copy_bbs): Update prototype.
>>         * tree-cfg.c (gimple_duplicate_sese_region):
>>         Add update_dominance argument.
>>         * tree-flow.h (gimple_duplicate_sese_region): Update prototype.
>>         * tree-ssa-loop-ch.c (copy_loop_headers): Update
>>         gimple_duplicate_sese_region call.
>>         * tree-vect-loop-manip.c (slpeel_tree_duplicate_loop_to_edge_cfg):
>>         Update copy_bbs call.
>>         * cfgloopmanip.c (duplicate_loop_to_header_edge): Ditto.
>>         * trans-mem.c (ipa_uninstrument_transaction): Ditto.
>
> So I'd change gimple_duplicate_sese_region to gimple_duplicate_seme region
> per comments from others.
>
> Where you document UPDATE_DOMINANCE, I'd add something like: When
> UPDATE_DOMINANCE is true, it is assumed that duplicating the region (or
> copying the blocks for copy_bbs) may change the dominator tree in ways that
> are not suitable for an incremental update and the caller is responsible for
> destroying and recomputing the dominator tree.
>
> Hmm, not terribly happy with that wording, but that gives you an idea of
> what I'm after.  When would someone set UPDATE_DOMINANCE to true and what
> are their responsibilities when they do so.
>
> Approved with the name change and a better comment for UPDATE_DOMINANCE.

Btw, the function does _not_ handle arbitrary SEME regions - it only handles
a single exit correctly and assumes no (SSA) data flows across the others.
So I'd rather not rename it.

Richard.

> Jeff
>