On Mon, 19 Jun 2023, Jan Hubicka wrote:
> Hi,
> this patch avoids unnecessary post dominator and update_ssa in phiprop.
>
> Bootstrapped/regtested x86_64-linux, OK?
>
> gcc/ChangeLog:
>
> * tree-ssa-phiprop.cc (propagate_with_phi): Add
> post_dominators_computed;
> compute post dominators lazilly.
> (const pass_data pass_data_phiprop): Remove TODO_update_ssa.
> (pass_phiprop::execute): Update; return TODO_update_ssa if something
> changed.
>
> diff --git a/gcc/tree-ssa-phiprop.cc b/gcc/tree-ssa-phiprop.cc
> index 3cb4900b6be..87e3a2ccf3a 100644
> --- a/gcc/tree-ssa-phiprop.cc
> +++ b/gcc/tree-ssa-phiprop.cc
> @@ -260,7 +260,7 @@ chk_uses (tree, tree *idx, void *data)
>
> static bool
> propagate_with_phi (basic_block bb, gphi *phi, struct phiprop_d *phivn,
> - size_t n)
> + size_t n, bool *post_dominators_computed)
> {
> tree ptr = PHI_RESULT (phi);
> gimple *use_stmt;
> @@ -324,6 +324,12 @@ propagate_with_phi (basic_block bb, gphi *phi, struct
> phiprop_d *phivn,
> gimple *def_stmt;
> tree vuse;
>
> + if (!*post_dominators_computed)
> + {
> + calculate_dominance_info (CDI_POST_DOMINATORS);
> + *post_dominators_computed = true;
I think you can save the parameter by using dom_info_available_p () here
and ...
> + }
> +
> /* Only replace loads in blocks that post-dominate the PHI node. That
> makes sure we don't end up speculating loads. */
> if (!dominated_by_p (CDI_POST_DOMINATORS,
> @@ -465,7 +471,7 @@ const pass_data pass_data_phiprop =
> 0, /* properties_provided */
> 0, /* properties_destroyed */
> 0, /* todo_flags_start */
> - TODO_update_ssa, /* todo_flags_finish */
> + 0, /* todo_flags_finish */
> };
>
> class pass_phiprop : public gimple_opt_pass
> @@ -490,9 +497,9 @@ pass_phiprop::execute (function *fun)
> gphi_iterator gsi;
> unsigned i;
> size_t n;
> + bool post_dominators_computed = false;
>
> calculate_dominance_info (CDI_DOMINATORS);
> - calculate_dominance_info (CDI_POST_DOMINATORS);
>
> n = num_ssa_names;
> phivn = XCNEWVEC (struct phiprop_d, n);
> @@ -508,7 +515,8 @@ pass_phiprop::execute (function *fun)
> if (bb_has_abnormal_pred (bb))
> continue;
> for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> - did_something |= propagate_with_phi (bb, gsi.phi (), phivn, n);
> + did_something |= propagate_with_phi (bb, gsi.phi (), phivn, n,
> + &post_dominators_computed);
> }
>
> if (did_something)
> @@ -516,9 +524,10 @@ pass_phiprop::execute (function *fun)
>
> free (phivn);
>
> - free_dominance_info (CDI_POST_DOMINATORS);
> + if (post_dominators_computed)
> + free_dominance_info (CDI_POST_DOMINATORS);
unconditionally free_dominance_info here.
> - return 0;
> + return did_something ? TODO_update_ssa : 0;
I guess that change is following general practice and good to catch
undesired changes (update_ssa will exit early when there's nothing
to do anyway).
OK with those changes.
