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
>
> 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 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.
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.
>>>>>>>
>>>>>>
>>>>
>>
