On Fri, Oct 09, 2026 at 11:17:54AM +0200, Richard Biener wrote:
> 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
OK
>
> > ...
> > 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.
OK
>
> > // 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 ()?
Just to check I got finalize() right:
// If ELIMINATE was passed to the constructor, finish the elimination.
// Otherwise, if INSERTED_EXPRS is passed, do PRE's full elimination.
unsigned finalize (bitmap inserted_exprs = NULL);
Sounds reasonable?
Aldy
