On Thu, Sep 13, 2012 at 4:00 PM, Eric Botcazou <ebotca...@adacore.com> wrote:
>> Indeed somewhat simple-minded - when originally fixing a similar testcase
>> (heh ...) I improved things by improving CFG cleanup to fold some more
>> conditions by looking at SSA defs, that improved things a lot. I also
>> thought the real fix should involve some scalar optimization on a
>> sub-range of the CFG. That should be easiest when performing the copy in
>> the first place - after all we keep copy tables and such for the purpose
>> of update-SSA so we might as well create a lattice from PHI nodes we
>> disassemble for use by copy_bb ...
>
> I also thought of similar approaches, but couldn't really come up with
> something satisfactory.
>
>> On the patch itself - can you call the simple CCP before we call
>> cleanup_tree_cfg () please?
>
> That was the original implementation (in fact, in the original implementation
> the simple CCP was conditionalized on canonicalize_loop_induction_variables,
> but you broke it a few days ago by moving the call to update_ssa :-)
>
> The problem is that, if it is moved to before cleanup_tree_cfg, then:
> 1) you have more basic blocks (15 vs 2 for the testcase),
> 2) you also need to do simple CCP for degenerate PHI nodes.
Yes - now cfg_cleanup does that (and it really shouldn't be its job). That
was the improvement I talked about - reducing the number of BBs a lot.
> That being said, I can certainly save in a bitmap the loop fathers for which
> canonicalize_loop_induction_variables unrolled a child and do the simple CPP
> (augmented for degenerate PHI nodes) on them between the calls to update_ssa
> and cleanup_tree_cfg.
Ah, indeed ;) Or just push struct loop of changed loops onto a stack.
>> We might get rid of that weirdo SSA lookup
>> there again then:
>>
>> static bool
>> cleanup_control_expr_graph (basic_block bb, gimple_stmt_iterator gsi)
>> {
>> ...
>> /* For conditions try harder and lookup single-argument
>> PHI nodes. Only do so from the same basic-block though
>> as other basic-blocks may be dead already. */
>> if (TREE_CODE (lhs) == SSA_NAME
>> && !name_registered_for_update_p (lhs))
>
> I'll investigate.
>
>> + FOR_EACH_IMM_USE_ON_STMT (use, iter)
>> + propagate_value (use, gimple_assign_rhs1 (stmt));
>> +
>> + fold_stmt_inplace (&use_stmt_gsi);
>> + update_stmt (use_stmt);
>>
>> Use SET_USE (use, rhs1) and cache gimple_assign_rhs1 somewhere.
>>
>> if (fold_stmt_inplace (&use_stmt_gsi))
>> update_stmt (use_stmt);
>
> OK, will adjust, thanks.
Thanks,
Richard.
> --
> Eric Botcazou
