Re: [PATCH 5/9] Come up with an abstraction.

["Martin Liška" <mliska@suse.cz>]

On 8/12/19 1:40 PM, Richard Biener wrote:
> On Mon, Aug 12, 2019 at 1:19 PM Martin Liška <mliska@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  <mliska@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).

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