Re: [PATCH 5/9] Come up with an abstraction.
On Wed, Sep 18, 2019 at 9:56 AM Martin Liška wrote:
>
> Hello.
>
> Ok, so the current IPA ICF transformation is being blocked by the
> patch 2/9 (about FIELD_DECL). I asked Honza for a help here.
> In the meantime, can you Richi make an opinion about the part 5 which
> is about the interaction in between old operand_equal_p and a new
> hook in IPA ICF?
+static operand_compare default_compare_instance;
+
+/* Conveinece wrapper around operand_compare class because usually we do
+ not need to play with the valueizer. */
+
+bool
+operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
+{
+ return default_compare_instance.operand_equal_p (arg0, arg1, flags);
+}
can we devirtualize this and thus further clone and devirtualize the
recursions in the default instance? if not does making the default
instance const help (you need to make the methods const, possibly
a good idea if that works for your ICF use as well?)
+ if (flag_checking && !(flags & OEP_NO_HASH_CHECK))
+{
better keep that in the caller (avoids the virtual call and also
then the function does what it says...).
Otherwise it looks OK. Note I'd really like to see the
overhead for the regular operand_equal_p calls being zero,
thus devirt is important here - but it should work with
IPA-CP of 'this'? But maybe the function is too big to
clone :/
Richard.
> Thanks,
> Martin
Re: [PATCH 5/9] Come up with an abstraction.
Hello. Ok, so the current IPA ICF transformation is being blocked by the patch 2/9 (about FIELD_DECL). I asked Honza for a help here. In the meantime, can you Richi make an opinion about the part 5 which is about the interaction in between old operand_equal_p and a new hook in IPA ICF? Thanks, Martin
Re: [PATCH 5/9] Come up with an abstraction.
t;>> +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".
LGTM, I'm sending updated for to address that.
>
>>> 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 th
Re: [PATCH 5/9] Come up with an abstraction.
On Wed, Aug 14, 2019 at 3:19 PM Martin Liška wrote:
>
> On 8/14/19 3:04 PM, Richard Biener wrote:
> > On Mon, Aug 12, 2019 at 3:56 PM Martin Liška wrote:
> >>
> >> On 8/12/19 2:43 PM, Richard Biener wrote:
> >>> On Mon, Aug 12, 2019 at 1:49 PM Martin Liška wrote:
>
> On 8/12/19 1:40 PM, Richard Biener wrote:
> > On Mon, Aug 12, 2019 at 1:19 PM Martin Liška 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
>
> * 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
Re: [PATCH 5/9] Come up with an abstraction.
On 8/14/19 3:04 PM, Richard Biener wrote:
> On Mon, Aug 12, 2019 at 3:56 PM Martin Liška wrote:
>>
>> On 8/12/19 2:43 PM, Richard Biener wrote:
>>> On Mon, Aug 12, 2019 at 1:49 PM Martin Liška wrote:
On 8/12/19 1:40 PM, Richard Biener wrote:
> On Mon, Aug 12, 2019 at 1:19 PM Martin Liška 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
* 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?
This is not going to work ..
> 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
Re: [PATCH 5/9] Come up with an abstraction.
On Mon, Aug 12, 2019 at 3:56 PM Martin Liška wrote:
>
> On 8/12/19 2:43 PM, Richard Biener wrote:
> > On Mon, Aug 12, 2019 at 1:49 PM Martin Liška wrote:
> >>
> >> On 8/12/19 1:40 PM, Richard Biener wrote:
> >>> On Mon, Aug 12, 2019 at 1:19 PM Martin Liška 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
> >>
> >> * 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
> >>
> >>>
Re: [PATCH 5/9] Come up with an abstraction.
On 8/12/19 2:43 PM, Richard Biener wrote:
> On Mon, Aug 12, 2019 at 1:49 PM Martin Liška wrote:
>>
>> On 8/12/19 1:40 PM, Richard Biener wrote:
>>> On Mon, Aug 12, 2019 at 1:19 PM Martin Liška 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
>>
>> * 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.
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.
>
>>
Re: [PATCH 5/9] Come up with an abstraction.
Hi, On Mon, 12 Aug 2019, Martin Liška wrote: > > 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). That's premature; why provide callbacks when it's always either NULL or a single value? What I meant is put code like this into operand_equal_p: if (flags & OE_FOR_ICF) op0 = oep_icf_valueize (op0); ... Less indirection, less confusion. Ciao, Michael.
Re: [PATCH 5/9] Come up with an abstraction.
On Mon, Aug 12, 2019 at 1:49 PM Martin Liška wrote:
>
> On 8/12/19 1:40 PM, Richard Biener wrote:
> > On Mon, Aug 12, 2019 at 1:19 PM Martin Liška 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
>
> * 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.
> >>>
> >>
>
Re: [PATCH 5/9] Come up with an abstraction.
On 8/12/19 1:40 PM, Richard Biener wrote: > On Mon, Aug 12, 2019 at 1:19 PM Martin Liška 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 * 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). 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. >>> >>
Re: [PATCH 5/9] Come up with an abstraction.
On Mon, Aug 12, 2019 at 1:19 PM Martin Liška 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 > >> > >> * 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. 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. > > >
Re: [PATCH 5/9] Come up with an abstraction.
On 8/8/19 5:55 PM, Michael Matz wrote: > Hi, > > On Mon, 10 Jun 2019, Martin Liska wrote: > >> 2019-07-24 Martin Liska >> >> * 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? 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. >
Re: [PATCH 5/9] Come up with an abstraction.
On Tue, Aug 6, 2019 at 5:44 PM Martin Liska wrote: > > > gcc/ChangeLog: Hum. I don't like the "abstraction" - how is it going to help you to not duplicate all the code? What's wrong with doing this all in ICF? Richard. > 2019-07-24 Martin Liska > > * 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. > * tree.c (add_expr): Move content to hash_operand. > --- > gcc/fold-const.c | 346 ++- > gcc/fold-const.h | 30 +++- > gcc/tree.c | 286 --- > 3 files changed, 372 insertions(+), 290 deletions(-) >
Re: [PATCH 5/9] Come up with an abstraction.
Hi, On Mon, 10 Jun 2019, Martin Liska wrote: > 2019-07-24 Martin Liska > > * 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. 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)? 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.
[PATCH 5/9] Come up with an abstraction.
gcc/ChangeLog:
2019-07-24 Martin Liska
* 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.
* tree.c (add_expr): Move content to hash_operand.
---
gcc/fold-const.c | 346 ++-
gcc/fold-const.h | 30 +++-
gcc/tree.c | 286 ---
3 files changed, 372 insertions(+), 290 deletions(-)
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 4bcde22ada7..087c450cace 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -2940,7 +2940,8 @@ combine_comparisons (location_t loc,
even if var is volatile. */
bool
-operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
+operand_compare::operand_equal_p (const_tree arg0, const_tree arg1,
+ unsigned int flags)
{
/* When checking, verify at the outermost operand_equal_p call that
if operand_equal_p returns non-zero then ARG0 and ARG1 has the same
@@ -2952,8 +2953,8 @@ operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
if (arg0 != arg1)
{
inchash::hash hstate0 (0), hstate1 (0);
- inchash::add_expr (arg0, hstate0, flags | OEP_HASH_CHECK);
- inchash::add_expr (arg1, hstate1, flags | OEP_HASH_CHECK);
+ hash_operand (arg0, hstate0, flags | OEP_HASH_CHECK);
+ hash_operand (arg1, hstate1, flags | OEP_HASH_CHECK);
hashval_t h0 = hstate0.end ();
hashval_t h1 = hstate1.end ();
gcc_assert (h0 == h1);
@@ -3094,6 +3095,12 @@ operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
|| (! TREE_SIDE_EFFECTS (arg0) && ! TREE_SIDE_EFFECTS (arg1
return true;
+ int val = operand_equal_valueize (arg0, arg1, flags);
+ if (val == 1)
+return 1;
+ if (val == 0)
+return 0;
+
/* Next handle constant cases, those for which we can return 1 even
if ONLY_CONST is set. */
if (TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1))
@@ -3605,6 +3612,339 @@ operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
#undef OP_SAME
#undef OP_SAME_WITH_NULL
+}
+
+/* Generate a hash value for an expression. This can be used iteratively
+ by passing a previous result as the HSTATE argument. */
+
+void
+operand_compare::hash_operand (const_tree t, inchash::hash ,
+ unsigned int flags)
+{
+ int i;
+ enum tree_code code;
+ enum tree_code_class tclass;
+
+ if (t == NULL_TREE || t == error_mark_node)
+{
+ hstate.merge_hash (0);
+ return;
+}
+
+ STRIP_ANY_LOCATION_WRAPPER (t);
+
+ if (!(flags & OEP_ADDRESS_OF))
+STRIP_NOPS (t);
+
+ code = TREE_CODE (t);
+
+ bool ret = hash_operand_valueize (t, hstate, flags);
+ if (ret)
+return;
+
+ switch (code)
+{
+/* Alas, constants aren't shared, so we can't rely on pointer
+ identity. */
+case VOID_CST:
+ hstate.merge_hash (0);
+ return;
+case INTEGER_CST:
+ gcc_checking_assert (!(flags & OEP_ADDRESS_OF));
+ for (i = 0; i < TREE_INT_CST_EXT_NUNITS (t); i++)
+ hstate.add_hwi (TREE_INT_CST_ELT (t, i));
+ return;
+case REAL_CST:
+ {
+ unsigned int val2;
+ if (!HONOR_SIGNED_ZEROS (t) && real_zerop (t))
+ val2 = rvc_zero;
+ else
+ val2 = real_hash (TREE_REAL_CST_PTR (t));
+ hstate.merge_hash (val2);
+ return;
+ }
+case FIXED_CST:
+ {
+ unsigned int val2 = fixed_hash (TREE_FIXED_CST_PTR (t));
+ hstate.merge_hash (val2);
+ return;
+ }
+case STRING_CST:
+ hstate.add ((const void *) TREE_STRING_POINTER (t),
+ TREE_STRING_LENGTH (t));
+ return;
+case COMPLEX_CST:
+ hash_operand (TREE_REALPART (t), hstate, flags);
+ hash_operand (TREE_IMAGPART (t), hstate, flags);
+ return;
+case VECTOR_CST:
+ {
+ hstate.add_int (VECTOR_CST_NPATTERNS (t));
+ hstate.add_int (VECTOR_CST_NELTS_PER_PATTERN (t));
+ unsigned int count = vector_cst_encoded_nelts (t);
+ for (unsigned int i = 0; i < count; ++i)
+ hash_operand (VECTOR_CST_ENCODED_ELT (t, i), hstate, flags);
+ return;
+ }
+case SSA_NAME:
+ /* We can just compare by pointer. */
+ hstate.add_hwi (SSA_NAME_VERSION (t));
+ return;
+case PLACEHOLDER_EXPR:
+ /* The node itself doesn't matter. */
+ return;
+case BLOCK:
+case OMP_CLAUSE:
+ /* Ignore. */
+ return;
+case TREE_LIST:
+ /* A list of expressions, for a CALL_EXPR or as the elements of a
+ VECTOR_CST. */
+ for (; t; t = TREE_CHAIN (t))
+ hash_operand (TREE_VALUE (t), hstate, flags);
+ return;
+case CONSTRUCTOR:
+ {
+ unsigned HOST_WIDE_INT idx;
+ tree field, value;
+ flags &=
