On Fri, 9 Oct 2026, Aldy Hernandez wrote:
> 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 do full elimination. INSERTED_EXPRS indicates insertions
// done by PRE.
> unsigned finalize (bitmap inserted_exprs = NULL);
So inserted_exprs cannot be used as flag, FRE runs in both iterating
(!eliminate) and non-iterating (eliminate) mode.
> Sounds reasonable?
Yes.
Richard.
> 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)
