On 6/25/2026 5:39 PM, Andrew Pinski wrote:
> So if we have:
> a = PHI <b, c>
> d = use(a)
>
> Try to optimize use(b) and use(c) [and the other values on the corresponding
> edge].
> This is for single uses as we don't want to increase the number of phis then
> this can be
> done at -Os. It allows for simple partial redundant caculation on the phi
> entries.
> Also this is cheapish since we only allow for single use and we remove the
> old phi
> and the old use statement too.
>
> This is an RFC because we have a few failures including a go front-end issue.
> The go front-end issue is debuged and a workaround patch is in PR 102138
> comment #11.
> I also ran into a case where pr46309-2.c started to fail; the analysis of the
> failure
> is located in PR 119988 (this is actually the second time I ran into this;
> the first
> time was with a similar patch but only handling conversions).
> gcc.dg/builtin-object-size-[12].c is also failing but forwprop is doing the
> correct
> thing but the testcase is not expecting to translate:
> ```
> if (x < 0)
> r = &a.a[9];
> else
> r = &a.c[1];
> ...
> t_1 = __builtin_object_size (r, 0)
> ```
> as r is only a single use in the BOS here. I am trying to figure out if that
> is
> ok and maybe we should still have a second use of r to force not doing the
> forwprop.
>
> I also have these testcase failures:
> +FAIL: gcc.dg/tree-ssa/phi_on_compare-4.c scan-tree-dump threadfull1 "Jumps
> threaded: 2"
> +FAIL: gcc.dg/tree-ssa/pr61839_1.c scan-tree-dump-times vrp1 "243048929 :
> 121524464" 1
> +FAIL: gcc.dg/tree-ssa/ssa-pre-32.c scan-tree-dump pre "# prephitmp_[0-9]+ =
> PHI <[xy]_[0-9]+\\\\(D\\\\)[^,]*, [xy]_[0-9]+\\\\(D\\\\)"
> +FAIL: gcc.dg/tree-ssa/ssa-pre-4.c scan-tree-dump-times pre "Eliminated: 1" 1
> +FAIL: gcc.dg/tree-ssa/ssa-pre-5.c scan-tree-dump-times pre "Eliminated: 1" 1
> But those just need to be fixed up as far as I can tell by disabling forwprop
> and maybe make a secondary testcase for the forwprop optimization now.
>
> I also have not collected data in any case to see how often this does
> something or loops.
>
> Also should this be forwprop or in phiprop?
> Also if forwprop, should instead phiprop be integrated into forwprop and
> removed?
> I am thinking phiprop is a style of forwprop but I could be wrong.
I think phiprop is just a specialized propagator. If you'd asked me
cold, I would have suggested forwprop rather than phiprop. No idea why
I did it in phiprop the first time around.
> Thanks,
> Andrea Pinski
>
> gcc/ChangeLog:
>
> * tree-ssa-forwprop.cc (phi_valuization): New function.
> (forwprop_phi): New function.
> (pass_forwprop::execute): Call phi_valuization on all phis of a bb until
> the no change is done.
>
> Signed-off-by: Andrew Pinski <[email protected]>
So I thought I went down the path of being more aggressive with this
kind of transformation eons ago and ultimately found it was not
profitable. But in reviewing the link (thanks for that) either I'm
thinking about a different problem or perhaps a much earlier version of
my patch.
> ---
>
> gcc/tree-ssa-forwprop.cc | 180 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 180 insertions(+)
>
> diff --git a/gcc/tree-ssa-forwprop.cc b/gcc/tree-ssa-forwprop.cc
> index 9bb001d2f63..431735494bf 100644
> --- a/gcc/tree-ssa-forwprop.cc
> +++ b/gcc/tree-ssa-forwprop.cc
> @@ -5272,6 +5272,168 @@ optimize_unreachable (basic_block bb)
> return ret;
> }
>
> +static edge phi_valueization_edge;
> +
> +static tree phi_valuization (tree val)
I think we use valueization rather than valuization. It's a nit, so if
you'd really prefer the other spelling, I won't complain.
> +
> + /* Handle only assignments and calls.
> + FIXME: Handle GIMPLE_COND. Producing a
> + boolean might improve jump threading. */
> + if (!is_a<gassign*> (use_stmt)
> + && !is_a<gcall*> (use_stmt))
> + return false;
Probably not strictly necessary to handle GIMPLE_COND. The interaction
with jump threading is probably mostly carried by getting more constant
values exposed in the PHIs. It's also the case the original was done
before we had Aldy's threading work IIRC. I guess I'd summarize as I
wouldn't worry too much about jump threading and presenting it with a
boolean vs an integer.
> +
> + /* Can only do this if we are replacing a SSA NAME lhs. */
> + tree lhs = gimple_get_lhs (use_stmt);
> + if (!lhs || TREE_CODE (lhs) != SSA_NAME)
> + return false;
> +
> + auto_vec<tree, 3> new_phi_args;
> + new_phi_args.reserve (gimple_phi_num_args (phi), true);
> + new_phi_args.quick_grow_cleared (gimple_phi_num_args (phi));
> + basic_block bb = gimple_bb (phi);
> + edge_iterator ei;
> + edge e;
> + FOR_EACH_EDGE (e, ei, bb->preds)
> + {
> + tree use = PHI_ARG_DEF_FROM_EDGE (phi, e);
> + /* This can only be done if we can prop the argument into the phi
> + result. An example where this matters is names that are
> + from abnormal edges. */
> + if (!may_propagate_copy (res, use))
> + return false;
> + phi_valueization_edge = e;
> + tree translated;
> +
> + translated = gimple_fold_stmt_to_constant_1 (use_stmt,
> phi_valuization);
> + if (!translated)
> + return false;
> +
> + /* Only invariants and ssa names can be used as phi arguments. */
> + if (!is_gimple_min_invariant (translated)
> + && TREE_CODE (translated) != SSA_NAME)
> + return false;
> +
> + /* If we get back an ssa name, make sure it
> + is valid for the phi argument, that is the
> + definition is dominating the edge. */
> + if (TREE_CODE (translated) == SSA_NAME)
> + {
> + basic_block tbb = gimple_bb (SSA_NAME_DEF_STMT (translated));
> + basic_block ebb = e->src;
> + /* The translated defining statement needs to be dominated
> + by the edge's src bb. */
> + if (tbb
> + && tbb != ebb
> + && !dominated_by_p (CDI_DOMINATORS, tbb, ebb))
> + return false;
Are your dominated_by_p block arguments reversed?
> @@ -5400,9 +5562,27 @@ pass_forwprop::execute (function *fun)
> if (may_propagate_copy (res, first))
> to_remove_defs.safe_push (SSA_NAME_VERSION (res));
> fwprop_set_lattice_val (res, first);
> + continue;
> }
> }
>
> + /* Try to forwprop PHIs and their arguments into
> + used once statements. */
> + if (!optimize_debug)
> + for (gphi_iterator si = gsi_start_phis (bb); !gsi_end_p (si);)
> + {
> + if (forwprop_phi (*si))
> + {
> + /* Start over if we replaced a phi as the iterator is
> + invalidated and we might have remved an usage for
> + another phi and also the new phi might have a single
> + usage. */
s/remved/removed/
Overall it seems like a reasonable track. I'd suggest getting it past
the RFC to an RFA.
jeff
