On Wed, Aug 14, 2019 at 3:19 PM Martin Liška <mli...@suse.cz> wrote: > > On 8/14/19 3:04 PM, Richard Biener wrote: > > On Mon, Aug 12, 2019 at 3:56 PM Martin Liška <mli...@suse.cz> wrote: > >> > >> On 8/12/19 2:43 PM, Richard Biener wrote: > >>> On Mon, Aug 12, 2019 at 1:49 PM Martin Liška <mli...@suse.cz> wrote: > >>>> > >>>> On 8/12/19 1:40 PM, Richard Biener wrote: > >>>>> On Mon, Aug 12, 2019 at 1:19 PM Martin Liška <mli...@suse.cz> wrote: > >>>>>> > >>>>>> On 8/8/19 5:55 PM, Michael Matz wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> On Mon, 10 Jun 2019, Martin Liska wrote: > >>>>>>> > >>>>>>>> 2019-07-24 Martin Liska <mli...@suse.cz> > >>>>>>>> > >>>>>>>> * fold-const.c (operand_equal_p): Rename to ... > >>>>>>>> (operand_compare::operand_equal_p): ... this. > >>>>>>>> (add_expr): Rename to ... > >>>>>>>> (operand_compare::hash_operand): ... this. > >>>>>>>> (operand_compare::operand_equal_valueize): Likewise. > >>>>>>>> (operand_compare::hash_operand_valueize): Likewise. > >>>>>>>> * fold-const.h (operand_equal_p): Set default > >>>>>>>> value for last argument. > >>>>>>>> (class operand_compare): New. > >>>>>>> > >>>>>>> Hmpf. A class without any data? That doesn't sound like a good > >>>>>>> design. > >>>>>> > >>>>>> Yes, the base class (current operand_equal_p) does not have a data. > >>>>>> But the ICF derive class has a data and e.g. > >>>>>> func_checker::operand_equal_valueize > >>>>>> will use m_label_bb_map.get (t1). Which are member data of class > >>>>>> func_checker. > >>>>>> > >>>>>>> You seem to need it only to have the possibility of virtual functions, > >>>>>>> i.e. fancy callbacks. AFAICS you only have one derived class, i.e. a > >>>>>>> simple distinction of two cases. What do you think about encoding the > >>>>>>> additional new (ICF) case in the (existing) 'flags' argument to > >>>>>>> operand_equal_p (and in case the ICF flag is set simply call the > >>>>>>> "callback" directly)? > >>>>>> > >>>>>> That's possible. I can add two more callbacks to the operand_equal_p > >>>>>> function > >>>>>> (hash_operand_valueize and operand_equal_valueize). > >>>>>> > >>>>>> Is Richi also supporting this approach? > >>>>> > >>>>> I still see no value in the abstraction since you invoke none of the > >>>>> (virtual) methods from the base class operand_equal_p. > >>>> > >>>> I call operand_equal_valueize (and hash_operand) from operand_equal_p. > >>>> These are then used in IPA ICF (patch 6/9). > >>> > >>> Ugh. I see you call that after > >>> > >>> if (TREE_CODE (arg0) != TREE_CODE (arg1)) > >>> { > >>> ... > >>> } > >>> else > >>> return false; > >>> } > >>> > >>> and also after > >>> > >>> /* Check equality of integer constants before bailing out due to > >>> precision differences. */ > >>> if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST) > >>> > >>> which means for arg0 == SSA_NAME and arg1 == INTEGER_CST you return false > >>> instead of valueizing arg0 to the possibly same or same "lose" value > >>> and returning true. > >> > >> Yes. ICF does not allow to have anything where TREE_CODEs do not match. > >> > >>> > >>> Also > >>> > >>> + int val = operand_equal_valueize (arg0, arg1, flags); > >>> + if (val == 1) > >>> + return 1; > >>> + if (val == 0) > >>> + return 0; > >>> > >>> suggests that you pass in arbirtrary trees for "valueization" but it > >>> isn't actually > >>> valueization that is performed but instead it should do an alternate > >>> comparison > >>> of arg0 and arg1 with valueization. Why's this done this way instead of > >>> sth like > >>> > >>> if (TREE_CODE (arg0) == SSA_NAME) > >>> arg0 = operand_equal_valueize (arg0, flags); > >>> if (TREE_CODE (arg1) == SSA_NAME) > >>> arg1 = operand_equal_valueize (arg1, flags); > >> > >> Because I want to be given a pair of trees about which the function > >> operand_equal_valueize returns match/no-match/dunno. > >> > >>> > >>> and why's this done with virtual functions rather than a callback that we > >>> can > >>> cheaply check for NULLness in the default implementation? > >> > >> I can transform it into a hook. But as mentioned I'll need two hooks. > >> > >>> > >>> So - what does ICF want to make "equal" that isn't equal normally and > >>> how's > >>> that "valueization"? > >> > >> E.g. for a FUNCTION_DECL, ICF always return true because it can only calls > >> the operand_equal_p after callgraph is compared. Similarly for LABEL_DECLs, > >> we have a map (m_label_bb_map). Please take a look at patch 6/9 in this > >> series. > > > > Hmm, ok, so you basically replace recursive calls to operand_equal_p with
_recursive calls_ > > > > operand_equal_valueize (t1, t2, 0) > > || operand_equal_p (t1, t2, 0) > > > > no? > > This is not going to work .. I wonder if class base { virtual operand_equal_p (tree a, tree b, int f); }; base::operand_equal_p (tree a, tree b, int f) { as-is now, recursing to virtual operand_equal_p } class deriv : public base { vritual operand_equal_p (tree a, tree b, int f); }; deriv::operand_equal_p (tree a, tree b, int f) { // just example if (TREE_CODE (a) == TREE_CODE (b) && TREE_CODE (a) == FUNCTION_DECL) return true; return base::operand_equal_p (tree a, tree b, int f); } would work? ICF would call deriv::operand_equal_p and base::operand_equal_p would recurse via the derived implementation. That at least is cleaner from the "looks". > > But the same could be achieved by actually making t1 and t2 equal > > according to operand_equal_p rules via the valueization hook? So replace > > FUNCTION_DECLs with their prevailing ones, LABEL_DECLs with theirs, etc. > > > > As given your abstraction is quite awkward to use, say, from value-numbering > > which knows how to "valueize" a single tree but doesn't compare things. > > > > To make it work for your case you'd valueize not only SSA names but also > > all DECL_P I guess. After all your operand_equal_valueize only does > > something for "leafs" but is called for all intermediate expressions as > > well. > > ... because I need to be called for all intermediate expression. One simple > example can be a ADDR_EXPR of a DECL. The first call will recursively call > operand_equal_p for the DECL and the DECL can be compared with > operand_equal_valueize > in ICF. > > Note that current ICF code is more complex than only selection of a canonical > form of a tree. > > I'm not saying the suggested API change is beautiful. But having a more > specific > equal hook seams to me a reasonable extension to current operand_equal_p. > Moreover, we'll be able to kill all the ICF duplicate comparison machinery. I wonder if all FUNCTION_DECL are really equal. If you just compare the callgraph you don't notice differences in the following (with disabled DSE/FRE to retain both stores to *dest) void fna(); void fnb(); void foo (void *dest) { *dest = (void *)fna; *dest = (void *)fnb; } void bar (void *dest) { *dest = (void *)fnb; *dest = (void *)fna; } and if you compare IPA refs you'd need to identify the ref stmts as the same? > Martin > > > > > Richard. > > > >> Thanks, > >> Martin > >> > >>> > >>> Thanks, > >>> Richard. > >>> > >>>> Martin > >>>> > >>>>> > >>>>> Richard. > >>>>> > >>>>>> Thanks, > >>>>>> Martin > >>>>>> > >>>>>>> IMHO that would also make the logic within > >>>>>>> operand_equal_p clearer, because you don't have to think about all the > >>>>>>> potential callback functions that might be called. > >>>>>>> > >>>>>>> > >>>>>>> Ciao, > >>>>>>> Michael. > >>>>>>> > >>>>>> > >>>> > >> >