Richard Biener <rguent...@suse.de> writes:
> On Wed, 11 Nov 2020, Jiufu Guo wrote:
>
>>
>> Thanks a lot for the sugguestion from previous mails.
>> The patch was updated accordingly.
>>
>> This updated patch propagates loop-closed PHIs them out after
>> loop_optimizer_finalize under a new introduced flag. At some cases,
>> to clean up loop-closed PHIs would save efforts of optimization passes
>> after loopdone.
>>
>> This patch passes bootstrap and regtest on ppc64le. Is this ok for trunk?
>
> Comments below
>
>> free_numbers_of_iterations_estimates (fn);
>>
>> + if (flag_clean_up_loop_closed_phi
>
> Sorry if there was miscommunication but I've not meant to add a
> new user-visible flag but instead a flag argument to loop_optimizer_finalize
> (as said, you can default it to false to only need to change the
> one in fini_loops)
Sorry for misunderstand.
Updated the patch.
>
>> + && loops_state_satisfies_p (fn, LOOP_CLOSED_SSA))
>> + {
>> + clean_up_loop_closed_phi (fn);
>> + loops_state_clear (fn, LOOP_CLOSED_SSA);
>> + }
>> +
......
>> + gphi *phi;
>> + tree rhs;
>> + tree lhs;
>> + gphi_iterator gsi;
>> + struct loop *loop;
>> + bool cfg_altered = false;
>> +
>> + /* Check dominator info before get loop-close PHIs from loop exits. */
>> + if (dom_info_state (CDI_DOMINATORS) != DOM_OK)
>
> Why?
>
As you said, loop_optimizer_finalize is also called from where dominator
info is not ready, e.g. called from: vrp(execute_vrp).
At there, loop exits info is not ready, and then
get_loop_exit_edges/get_loop_body_with_size function does not work.
>> + /* Walk over loop in function. */
>> + FOR_EACH_LOOP_FN (fun, loop, 0)
>> + {
>> + /* Check each exit edege of loop. */
>> + auto_vec<edge> exits = get_loop_exit_edges (loop);
>> + FOR_EACH_VEC_ELT (exits, i, e)
>> + if (single_pred_p (e->dest))
>> + /* Walk over loop-closed PHIs. */
>> + for (gsi = gsi_start_phis (e->dest); !gsi_end_p (gsi);)
>> + {
>> + phi = gsi.phi ();
>> + rhs = degenerate_phi_result (phi);
>
> You have a single predecessor, thus the result is always degenerate.
>
>> + lhs = gimple_phi_result (phi);
>> +
>> + if (rhs && may_propagate_copy (lhs, rhs))
>> + {
>> + gimple_stmt_iterator psi = gsi;
>> + /* Advance the iterator before stmt is removed. */
>> + gsi_next (&gsi);
>
> remove_phi_node should take care of this, you shouldn't need this
> (just do not advance the iterator when you remove the PHI node).
>
Yeap, get it! Thanks, will update the patch accordingly.
>> +
......
>> +
>> + replace_uses_by (lhs, rhs);
>> + remove_phi_node (&psi, true);
>> + cfg_altered = true;
>
> in the end the return value is unused but I think we should avoid
> altering the CFG since doing so requires it to be cleaned up for
> unreachable blocks. That means to open-code replace_uses_by as
>
> imm_use_iterator imm_iter;
> use_operand_p use;
> gimple *stmt;
> FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name)
> {
> FOR_EACH_IMM_USE_ON_STMT (use, imm_iter)
> replace_exp (use, val);
> update_stmt (stmt);
> }
Thansk! This could also save some code in replace_uses_by.
BR.
Jiufu Guo
>
> Thanks,
> Richard.
>
>> + }
>> + else
>> + gsi_next (&gsi);
>> + }
>> + }
>> +
>> + return cfg_altered;
>> +}
>>
