This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
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 <mliska@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 <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).
>
> 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.
>>>>>
>>>>
>>