On Mon, Jun 29, 2026 at 3:15 PM Jeffrey Law <[email protected]> wrote:
>
> 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.
So the reasoning, I was thinking about handling a GIMPLE_COND here is
spaceship_replacement in phiopt handles GIMPLE_COND.
Maybe the jump threading is enough here and I don't need to handle
GIMPLE_COND and we can still remove spaceship_replacement after all.
Note spaceship_replacement is a special case of this patch plus some
extra phi conversions. So removing it would make phiopt less big and
don't need to handle as much.
It also looks there needs to be some more handling outside of
spaceship_replacement to fix up the cases there after this patch. so I
will work on that first.
PR 91425 and PR 53806 (and maybe others).
Thanks,
Andrea
>
> > +
> > + /* 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
