On Fri, 9 Oct 2026, Aldy Hernandez wrote:
> On Fri, Oct 09, 2026 at 09:51:01AM +0200, Richard Biener wrote:
> > On Thu, 8 Oct 2026, Aldy Hernandez wrote:
> >
> > > RPO VN is driven through a couple free functions that we can abstract
> > > out to a class to make things a bit tidier, and to make it easier to
> > > call the back threader from within FRE to get value numbering for
> > > free.
> > >
> > > Make it an object that we can later sub-class into a path aware VN
> > > object.
> > >
> > > OK for trunk?
> >
> > Hmm. I think .walk () should be part of constructing (it's a bad
> > name anyway), that makes it obvious it's a single-shot thing.
> >
> > Overall it feels a bit of a half-way transition, merely covering
> > parts of the API, so I don't think it's much of an improvement.
> >
> > As it is more like a "VN driver" class, maybe naming it accordingly
> > and _not_ move functions into it as member functions, just encapsulate
> > the toplevel flow, construct (with value-number), (optional) eliminate,
> > destruction.
> >
> > So I think this patch goes too far and too little.
> >
> > Not knowing how you'd use it exactly doesn't help making a constructive
> > suggestion, of course ;)
>
> Ughhh sorry, reviewing blind is a bit unfair :). I should provide
> some more context.
>
> The idea is that the value numbers outlive the walk, and the path_vn
> subclass value numbers each candidate path's blocks again using the
> walk's rpo_elim and RPO map.
>
> If I understand your suggestion, how about something like this:
>
> class vn_driver
> {
> public:
> vn_driver (function *, bool iterate, bool eliminate,
> vn_lookup_kind = VN_WALKREWRITE);
> vn_driver (function *, edge entry, bitmap exit_bbs, bool iterate,
> bool eliminate, bool skip_entry_phis, vn_lookup_kind);
> virtual ~vn_driver ();
> unsigned todo () const;
> // Split out the last part of elimination so the threader can see IL
> // consistent with the value numbers. Otherwise it can trip
> // over released SSA names, or a CFG the VNs no longer describe.
> unsigned eliminate_finish ();
> protected:
> // path_vn will use these
> rpo_elim *m_avail;
> int *m_bb_to_rpo;
> };
>
> class path_vn : public vn_driver
> {
> public:
> bool reset_path (const vec<basic_block> &path);
> void clear_path ();
> tree valnum (tree name);
> };
>
> do_rpo_vn_1 would need to be passed down rpo_elim and bb_to_rpo from
> the driver so path_vn could use them.
>
> The driver's use would be something like this:
>
> // do_rpo_vn:
> vn_driver vn (fn, entry, exit_bbs, iterate, eliminate,
> skip_entry_phis, kind);
> return vn.todo ();
>
> // PRE
> vn_driver vn (fun, true, false, VN_WALK);
> // What's left of run_rpo_vn
> assign_value_ids ();
this is part of value-numbering, so I'd add a bool
arg to the CTOR specifying want_value_ids
> ...
> todo |= eliminate_with_rpo_vn (inserted_exprs);
so I'd say this is part of the driver, so
todo |= vn.eliminate (inserted_exprs);
but have the method just call eliminate_with_rpo_vn. And
assert that 'eliminate' was specified false at CTOR time.
> // FRE + threading
> path_vn vn (fun, iterate_p, true);
> // The path solver calls reset_path and valnum
> threader.search (vn);
> todo = vn.todo () | vn.eliminate_finish ();
maybe just
todo = vn.finalize ();
which either does eliminate_finish or full eliminate? So no
need for .eliminate () or .eliminate_finish () or .todo ()?
> todo |= threader.apply ();
>
> Is that closer to what you had in mind?
Yes, I think so.
> Aldy
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri; (HRB 36809, AG Nuernberg)
