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
operand_equal_valueize (t1, t2, 0)
|| operand_equal_p (t1, t2, 0)
no? 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.
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.
> >>>>>
> >>>>
> >>
>
