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

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

On Mon, Jan 5, 2015 at 2:12 PM, Martin LiÅka <mliska@suse.cz> wrote:
> On 12/19/2014 12:04 PM, Richard Biener wrote:
>>
>> On Thu, Dec 18, 2014 at 6:38 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.
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>
>>> Hi.
>>>
>>> There's second part of the patch set.
>>
>>
>> It still has similar issues:
>>
>> +/* Function compare for equality given memory operands T1 and T2.  */
>> +
>> +bool
>> +func_checker::compare_memory_operand (tree t1, tree t2)
>> +{
>> +  if (!t1 && !t2)
>> +    return true;
>> +  else if (!t1 || !t2)
>> +    return false;
>> +
>> +  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
>> +    return return_false_with_msg ("different operand volatility");
>> +
>> +  bool source_is_memop = DECL_P (t1) || INDIRECT_REF_P (t1)
>> +                        || TREE_CODE (t1) == MEM_REF
>> +                        || TREE_CODE (t1) == TARGET_MEM_REF;
>> +
>> +  bool target_is_memop = DECL_P (t2) || INDIRECT_REF_P (t2)
>> +                        || TREE_CODE (t2) == MEM_REF
>> +                        || TREE_CODE (t2) == TARGET_MEM_REF;
>> +
>> +  /* Compare alias sets for memory operands.  */
>> +  if (source_is_memop && target_is_memop)
>> +    {
>> +      ao_ref r1, r2;
>> +      ao_ref_init (&r1, t1);
>> +      ao_ref_init (&r2, t2);
>>
>> the *_is_memop tests need to work on the memory reference base.  Thus
>> simply move the ao_ref_init's before those checks and do them on
>> the result of ao_ref_base ().
>>
>> Similarly the volatile check can be put into the if (source_is_memop
>> && target_is_memop) block, volatileness doesn't matter for non-memory
>> operands.
>>
>> Ok with that changes.
>>
>> Thanks,
>> Richard.
>
>
> Hello Richard.
>
> Just for being sure, there's final version of the patch that passes
> regression tests.
>
> Ready for trunk in this shape?

Ok.

Thanks,
Richard.

> Thanks,
> Martin
>