diff mbox

IPA ICF: refactoring + fix for PR ipa/63569

Message ID 5492D924.6080504@suse.cz
State New
Headers show

Commit Message

Martin Liška Dec. 18, 2014, 1:39 p.m. UTC 
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?

I'm going to apply your notes that are orthogonal to the PR.

Thanks,
Martin

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

Comments

Richard Biener Dec. 19, 2014, 10:59 a.m. UTC | #1 
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
>>>
>>>
>>>
>
diff mbox

Patch

From b984eaae263eedc8beb2413f4739d3574f46d63d Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Thu, 18 Dec 2014 14:16:12 +0100
Subject: [PATCH 1/2] Fix for PR ipa/63569.

gcc/testsuite/ChangeLog:

2014-12-18  Martin Liska  <mliska@suse.cz>

	PR ipa/63569
	* gcc.dg/ipa/pr63569.c: New test.

gcc/ChangeLog:

2014-12-18  Martin Liska  <mliska@suse.cz>

	PR ipa/63569
	* ipa-icf-gimple.c (func_checker::compare_operand): Add missing
	comparison for volatile flag.
---
 gcc/ipa-icf-gimple.c               |  3 +++
 gcc/testsuite/gcc.dg/ipa/pr63569.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr63569.c

diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index ec0290a..fa2c353 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -230,6 +230,9 @@  func_checker::compare_operand (tree t1, tree t2)
   tree tt1 = TREE_TYPE (t1);
   tree tt2 = TREE_TYPE (t2);
 
+  if (TREE_THIS_VOLATILE (t1) != TREE_THIS_VOLATILE (t2))
+    return return_false_with_msg ("different operand volatility");
+
   if (!func_checker::compatible_types_p (tt1, tt2))
     return false;
 
diff --git a/gcc/testsuite/gcc.dg/ipa/pr63569.c b/gcc/testsuite/gcc.dg/ipa/pr63569.c
new file mode 100644
index 0000000..8bd5c1f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr63569.c
@@ -0,0 +1,32 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf-details"  } */
+
+static int f(int t, int *a) __attribute__((noinline));
+
+static int g(int t, volatile int *a) __attribute__((noinline));
+static int g(int t, volatile int *a)
+{
+  int i;
+  int tt = 0;
+  for(i=0;i<t;i++)
+    tt += *a;
+  return tt;
+}
+static int f(int t, int *a)
+{
+  int i;
+  int tt = 0;
+  for(i=0;i<t;i++)
+    tt += *a;
+  return tt;
+}
+
+
+int h(int t, int *a)
+{
+  return f(t, a) + g(t, a);
+}
+
+/* { dg-final { scan-ipa-dump "different operand volatility" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Equal symbols: 0" "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
-- 
2.1.2