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

[Richard Biener]

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.

> Thank you,
> Martin
>
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> Martin
>
>