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.
Most likely because it was suggested a few times to do it in phiprop. > > 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. I think there has been more improvements happening that has allowed for the more aggressive folding to work better now. Even a version which I added to a bug report was only for casts. I think I had noticed some regressions back then too. > > > > --- > > > > 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. valueization is what others uses so I will change that. > > + > > + /* 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. Yes it is not really about jump threading. And not even boolean vs integer but boolean vs other type; and most likely will cause a phiopt to happen. Example of that: https://godbolt.org/z/jrdov6a59 (Will add a testcase when I add support for GIMPLE_COND). > > > + > > + /* 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? Most likely; I have to double check. > > > @@ -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
