On Thu, Sep 22, 2022 at 2:34 PM Richard Biener <richard.guent...@gmail.com> wrote: > > On Wed, Sep 7, 2022 at 9:30 AM Di Zhao OS <diz...@os.amperecomputing.com> > wrote: > > > > Gentle ping again. > > So I got the chance to review the change again on the travel to GNU > Cauldron 2022. > > There's quite some factoring / moving of stuff in the patch. I've > already pushed to trunk > a change that factores out can_track_predicate_on_edge (your > vn_tracking_edge), > factoring out is_vn_valid_at_bb also looks useful, so I'll followup > with a similar change. > > I'm going to attach a commented quoted patch (because that's what I > produced during > the travel). An overall comment (also in that attachment) would be > > "why are equivalences recorded in the expression hash table at all? Are > they not predicated values of an SSA name and thus should be a > vn_pval chain in vn_ssa_aux itself? > > conditional equivalences are expensive to handle (so are the existing > predicated values which I do not like too much and which, frankly, I've > probably designed too general - ATM we only ever insert predicated > values 'true' and 'false' which could be used to simplify a lot of logic > but would break this patch?) > > At some point I wanted to see whether we can use ranger relations > for all of this. > > Then, for "true" equivalence tracking it might be interesting to explore > "path value numbering", aka allow revisiting code from the equivalence > op defs to the equivalence producing edge(s) with the equivalence fully > reflected in the value number. The interesting thing might be that we > can track whether there's any equivalence on the side and based on > use heuristic decide whether that's going to pay off. It might be also > possible to re-use this to improve jump threading costing. If we'd be > able to "fork" the VN state we could re-run from the later definition > of an equivalence to the point it is established. > > So, overall I wasn't able to get at what this patch will catch and what > it will not catch - that is, to what extent equivalences affect > previously and future recorded expressions. Plus the implementation > feels like it bolts on the wrong place. > > As I'm not happy with my predicated values implementation either I'm > of course a bit biased here (note the implementation was mostly added > to avoid regressions with respect to the previous VN implementation > and I should probably make it less general and more optimized - but > as said, using ranger might be an option here). > > You have one testcase, ssa-fre-102.c, that seems to require VN > with equivalences, the others should be catched by rangers relation > handling, no?" > > I've looked into using ranger for what the existing predicated value > handling does, plus catch more cases transparently. I'm not sure > rangers equivalences handling is a good fit so to handle those an > approach like yours might be necessary. Note I'm not really happy > about the patch as-is (nor I am happy about what I implemented > with predicated values - sorry for that). I'm not even sure equivalences > can be handled "nicely" :/
Meh, I said I would attach something. Here it is. Richard.
review
Description: Binary data
