Re: [PATCH] IPA ICF: refactoring + fix for PR ipa/63569

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Dec 18, 2014 at 2:39 PM, Martin LiÅka <mliska@suse.cz> wrote:
> On 12/17/2014 04:23 PM, Richard Biener wrote:
>>
>> On Wed, Dec 17, 2014 at 12:17 PM, Martin LiÅka <mliska@suse.cz> wrote:
>>>
>>> On 12/11/2014 01:37 PM, Richard Biener wrote:
>>>>
>>>>
>>>> On Wed, Dec 10, 2014 at 1:18 PM, Martin LiÅka <mliska@suse.cz> wrote:
>>>>>
>>>>>
>>>>> Hello.
>>>>>
>>>>> As suggested by Richard, I split compare_operand functions to various
>>>>> functions
>>>>> related to a specific comparison. Apart from that I added fast check
>>>>> for
>>>>> volatility flag that caused miscompilation mentioned in PR63569.
>>>>>
>>>>> Patch can bootstrap on x86_64-linux-pc without any regression seen and
>>>>> I
>>>>> was
>>>>> able to build Firefox with LTO.
>>>>>
>>>>> Ready for trunk?
>>>>
>>>>
>>>>
>>>> Hmm, I don't think the dispatch to compare_memory_operand is at the
>>>> correct place.  It should be called from places where currently
>>>> compare_operand is called and it should recurse to compare_operand.
>>>> That is, it is more "high-level".
>>>>
>>>> Can you please fix the volatile issue separately?  It's also not
>>>> necessary
>>>> to do that check on every operand but just on memory operands.
>>>
>>>
>>>
>>> Hello Richard.
>>>
>>> I hope the following patch is the finally the right approach we want to
>>> do
>>> ;)
>>> In the first patch, I did just refactoring for high-level memory operand
>>> comparison and the second of fixes problem connected to missing volatile
>>> attribute comparison.
>>>
>>> Patch was retested and can bootstrap.
>>>
>>> What do you think about it?
>>
>>
>> +bool
>> +func_checker::compare_cst_or_decl (tree t1, tree t2)
>> +{
>> ...
>> +    case INTEGER_CST:
>> +      {
>> +       ret = compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2))
>> +             && wi::to_offset  (t1) == wi::to_offset  (t2);
>> +
>> +       return return_with_debug (ret);
>> +      }
>> +    case COMPLEX_CST:
>> +    case VECTOR_CST:
>> +    case STRING_CST:
>> +    case REAL_CST:
>> +      {
>> +       ret = operand_equal_p (t1, t2, OEP_ONLY_CONST);
>> +       return return_with_debug (ret);
>>
>> why does the type matter for INTEGER_CSTs but not for other constants?
>> Also comparing INTEGER_CSTs via to_offset is certainly wrong.  Please
>> use tree_int_cst_equal_p (t1, t2) instead, or operand_equal_p which would
>> end up calling tree_int_cst_equal_p.  That said, I'd have commonized all
>> _CST nodes by
>>
>>    ret = compatible_types_p (....) && operand_equal_p (...);
>>
>> +    case CONST_DECL:
>> +    case BIT_FIELD_REF:
>> +      {
>>
>> I'm quite sure BIT_FIELD_REF is spurious here.
>>
>> Now to the "meat" ...
>>
>> +      tree load1 = get_base_loadstore (t1, false);
>> +      tree load2 = get_base_loadstore (t2, false);
>> +
>> +      if (load1 && load2 && !compare_memory_operand (t1, t2))
>> +       return return_false_with_msg ("memory operands are different");
>> +      else if ((load1 && !load2) || (!load1 && load2))
>> +       return return_false_with_msg ("");
>> +
>>
>> and the similar code for assigns.  The way you introduce the
>> unpack_handled_component flag to get_base_loadstore makes
>> it treat x.field as non-memory operand.  That's wrong.  I wonder
>> why you think you needed that.  Why does
>>
>>    tree load1= get_base_loadstore (t1);
>>
>> not work?  Btw, I'd have avoided get_base_loadstore and simply did
>>
>>    ao_ref_init (&r1, t1);
>>    ao_ref_init (&r2, t2);
>>    tree base1 = ao_ref_base (&r1);
>>    tree base2 = ao_ref_base (&r2);
>>    if ((DECL_P (base1) || (TREE_CODE (base1) == MEM_REF || TREE_CODE
>> (base1) == TARGET_MEM_REF))
>>       && (DECL_P (base2) || (...))
>>     ...
>>
>> or rather put this into compare_memory_operand and call that on all
>> operands
>> that may be memory (TREE_CODE () != SSA_NAME && !is_gimple_min_invariant
>> ()).
>>
>> I also miss where you handle memory operands on the LHS for calls
>> and for assignments.
>>
>> The compare_ssa_name refactoring part is ok to install.
>>
>> Can you please fix the volatile bug before the refactoring as it looks
>> like
>> we're going to iterate some more here unless I can find the time to give
>> it a quick try myself.
>
>
> Hi.
>
> Sure, there's minimalistic patch which fixes the PR.
> Can I install this change?

Ok.

Thanks,
Richard.

> I'm going to apply your notes that are orthogonal to the PR.
>
> Thanks,
> Martin
>
>
>>
>> Thanks,
>> Richard.
>>
>>> Thank you,
>>> Martin
>>>
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> Thanks,
>>>>> Martin
>>>
>>>
>>>
>