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.
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);
and why's this done with virtual functions rather than a callback that we can
cheaply check for NULLness in the default implementation?
So - what does ICF want to make "equal" that isn't equal normally and how's
that "valueization"?
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.
> >>>
> >>
>
