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