[4.9] PR 62146

[4.9] PR 62146

[Easwaran Raman]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

This patch deletes REG_EQUAL note when a src register is replaced by a
constant in an assignment. This is to prevent spurious equivalences
between the constant and the expression in the REG_EQUAL note. In the
bug reported in PR 62146, an assignment in one branch (which is
actually dead) of an IF statement has a REG_EQUAL note equating a
register with an expression. Conditional copy propagation replaces the
register with 0. The instruction is hoisted above the branch
subsequently and then the value 0 is equated with the expression in
the REG_EQUAL. Is this ok for 4.9 branch if all tests pass?

This patch looks applicable to trunk as well, but I don't have a test
case to reproduce the issue in trunk.


ChangeLog:

2014-08-25  Easwaran Raman  <eraman@google.com>

        PR rtl-optimization/62146
        * cprop.c (try_replace_reg): Remove REG_EQUAL note when a constant is
        propagated into the src of an assignment.

testsuite/ChangeLog

2014-08-25  Easwaran Raman  <eraman@google.com>

        PR rtl-optimization/62146
        * testsuite/g++.dg/opt/pr62146.C: New.

[-- Attachment #2: pr62146.patch --]
[-- Type: text/x-patch, Size: 1983 bytes --]

Index: gcc/testsuite/g++.dg/opt/pr62146.C
===================================================================
--- gcc/testsuite/g++.dg/opt/pr62146.C	(revision 0)
+++ gcc/testsuite/g++.dg/opt/pr62146.C	(revision 0)
@@ -0,0 +1,51 @@
+/* PR rtl-optimization/62146 */
+/* { dg-do compile } */
+/* { dg-options "-O2 " } */
+class F
+{
+public:
+    virtual ~ F ();
+};
+template < class CL > class G:public F
+{
+    int *member_;
+public:
+    G ( int *b): member_ (0)
+    {
+    }
+};
+
+class D
+{
+public:
+    template < class CL > void RegisterNonTagCallback (int,
+            void (CL::
+                  *p3) ())
+    {
+        InternalRegisterNonTag (p3 ? new G < CL > ( 0) : 0);
+    } void InternalRegisterNonTag (F *);
+};
+
+void fn1 ();
+class C1
+{
+    void  foo();
+    class TokenType
+    {
+    public:
+        void AddToken ()
+        {
+        }
+    };
+    C1::TokenType bar_t;
+};
+D a;
+void C1::foo()
+{
+    if (&bar_t)
+        fn1 ();
+    for (int i = 0; i < sizeof 0; ++i)
+        a.RegisterNonTagCallback (0, &TokenType::AddToken);
+}
+
+/* { dg-final { scan-assembler-not "mov.*_ZN2C19TokenType8AddTokenEv, .\\\(" } } */
Index: gcc/cprop.c
===================================================================
--- gcc/cprop.c	(revision 214472)
+++ gcc/cprop.c	(working copy)
@@ -790,8 +790,11 @@ try_replace_reg (rtx from, rtx to, rtx insn)
   /* REG_EQUAL may get simplified into register.
      We don't allow that. Remove that note. This code ought
      not to happen, because previous code ought to synthesize
-     reg-reg move, but be on the safe side.  */
-  if (note && REG_NOTE_KIND (note) == REG_EQUAL && REG_P (XEXP (note, 0)))
+     reg-reg move, but be on the safe side. The REG_EQUAL note is
+     also removed when the source is a constant.  */
+  if (note && REG_NOTE_KIND (note) == REG_EQUAL
+      && (REG_P (XEXP (note, 0))
+          || (set && CONSTANT_P (SET_SRC (set)))))
     remove_note (insn, note);
 
   return success;

Re: [4.9] PR 62146

[Easwaran Raman]

On Mon, Aug 25, 2014 at 3:42 PM, Easwaran Raman <eraman@google.com> wrote:
> This patch deletes REG_EQUAL note when a src register is replaced by a
> constant in an assignment. This is to prevent spurious equivalences
> between the constant and the expression in the REG_EQUAL note. In the
> bug reported in PR 62146, an assignment in one branch (which is
> actually dead) of an IF statement has a REG_EQUAL note equating a
> register with an expression. Conditional copy propagation replaces the
> register with 0. The instruction is hoisted above the branch
> subsequently and then the value 0 is equated with the expression in
> the REG_EQUAL. Is this ok for 4.9 branch if all tests pass?
>
> This patch looks applicable to trunk as well, but I don't have a test
> case to reproduce the issue in trunk.
>
>
> ChangeLog:
>
> 2014-08-25  Easwaran Raman  <eraman@google.com>
>
>         PR rtl-optimization/62146
>         * cprop.c (try_replace_reg): Remove REG_EQUAL note when a constant is
>         propagated into the src of an assignment.
>
> testsuite/ChangeLog
>
> 2014-08-25  Easwaran Raman  <eraman@google.com>
>
>         PR rtl-optimization/62146
>         * testsuite/g++.dg/opt/pr62146.C: New.

Re: [4.9] PR 62146

[Jeff Law]

On 08/25/14 16:42, Easwaran Raman wrote:
> This patch deletes REG_EQUAL note when a src register is replaced by a
> constant in an assignment. This is to prevent spurious equivalences
> between the constant and the expression in the REG_EQUAL note. In the
> bug reported in PR 62146, an assignment in one branch (which is
> actually dead) of an IF statement has a REG_EQUAL note equating a
> register with an expression. Conditional copy propagation replaces the
> register with 0. The instruction is hoisted above the branch
> subsequently and then the value 0 is equated with the expression in
> the REG_EQUAL. Is this ok for 4.9 branch if all tests pass?
>
> This patch looks applicable to trunk as well, but I don't have a test
> case to reproduce the issue in trunk.
Something doesn't feel right with this patch.  It seems to me the real 
problem is when when hoist the insn with the note.  If the equivalence 
implied by the note is no longer valid at the insn's new location, then 
the note needs to be removed.

Now determining if the note is no longer valid at the new location may 
prove difficult ;-)  You'd probably have to know why the note was 
created, how it was changed, etc.  So I suspect the right thing to do is 
just remove REG_EQUAL notes on any insns we hoist in this manner.

Jeff

Re: [4.9] PR 62146

[Easwaran Raman]

It turns out that the REG_EQUAL note is removed on a hoisted
instruction (relevant code is in dead_or_predicable in ifcvt.c) if the
source of the move instruction is not a function invariant. In this
case, the source is a function invariant (constant) and so that
doesn't kick in. I don't understand why this exemption for function
invariant is there and the original thread in
https://gcc.gnu.org/ml/gcc/2005-05/msg01710.html doesn't explain
either. Should I just remove the REG_EQUAL notes of all hoisted
instructions or are there cases where it is safe to leave the note?

Thanks,
Easwaran



On Fri, Aug 29, 2014 at 1:06 PM, Jeff Law <law@redhat.com> wrote:
> On 08/25/14 16:42, Easwaran Raman wrote:
>>
>> This patch deletes REG_EQUAL note when a src register is replaced by a
>> constant in an assignment. This is to prevent spurious equivalences
>> between the constant and the expression in the REG_EQUAL note. In the
>> bug reported in PR 62146, an assignment in one branch (which is
>> actually dead) of an IF statement has a REG_EQUAL note equating a
>> register with an expression. Conditional copy propagation replaces the
>> register with 0. The instruction is hoisted above the branch
>> subsequently and then the value 0 is equated with the expression in
>> the REG_EQUAL. Is this ok for 4.9 branch if all tests pass?
>>
>> This patch looks applicable to trunk as well, but I don't have a test
>> case to reproduce the issue in trunk.
>
> Something doesn't feel right with this patch.  It seems to me the real
> problem is when when hoist the insn with the note.  If the equivalence
> implied by the note is no longer valid at the insn's new location, then the
> note needs to be removed.
>
> Now determining if the note is no longer valid at the new location may prove
> difficult ;-)  You'd probably have to know why the note was created, how it
> was changed, etc.  So I suspect the right thing to do is just remove
> REG_EQUAL notes on any insns we hoist in this manner.
>
> Jeff

Re: [4.9] PR 62146

[Jeff Law]

On 09/02/14 12:52, Easwaran Raman wrote:
> It turns out that the REG_EQUAL note is removed on a hoisted
> instruction (relevant code is in dead_or_predicable in ifcvt.c) if the
> source of the move instruction is not a function invariant. In this
> case, the source is a function invariant (constant) and so that
> doesn't kick in. I don't understand why this exemption for function
> invariant is there and the original thread in
> https://gcc.gnu.org/ml/gcc/2005-05/msg01710.html doesn't explain
> either. Should I just remove the REG_EQUAL notes of all hoisted
> instructions or are there cases where it is safe to leave the note?
I suspect it was left in in an attempt to keep as many REG_EQUAL notes 
as possible as the notes can help later optimizations.  But even those 
equivalences are not necessarily safe.

I'm pretty sure the right thing to do here is just drop the note 
regardless of whether or not its an invariant.

jeff

Re: [4.9] PR 62146

[Easwaran Raman]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

I've attached the revised patch. Bootstrapped and no test regressions
on x86_64/linux with 4.9 branch. Ok for 4.9 branch? While the bug
doesn't show up in trunk, seems obvious that this should go to trunk
as well. Is it ok for trunk if tests pass?

Btw, is g++.dg/opt the right directory for the test to go?

-----

ChangeLog

2014-09-04  Easwaran Raman  <eraman@google.com>

        PR rtl-optimization/62146
        * ifcvt.c (dead_or_predicable): Make removal of REG_EQUAL note of
        hoisted instruction unconditional.

2014-09-04  Easwaran Raman  <eraman@google.com>

        PR rtl-optimization/62146
        * testsuite/g++.dg/opt/pr62146.C: New.


On Wed, Sep 3, 2014 at 11:36 AM, Jeff Law <law@redhat.com> wrote:
> On 09/02/14 12:52, Easwaran Raman wrote:
>>
>> It turns out that the REG_EQUAL note is removed on a hoisted
>> instruction (relevant code is in dead_or_predicable in ifcvt.c) if the
>> source of the move instruction is not a function invariant. In this
>> case, the source is a function invariant (constant) and so that
>> doesn't kick in. I don't understand why this exemption for function
>> invariant is there and the original thread in
>> https://gcc.gnu.org/ml/gcc/2005-05/msg01710.html doesn't explain
>> either. Should I just remove the REG_EQUAL notes of all hoisted
>> instructions or are there cases where it is safe to leave the note?
>
> I suspect it was left in in an attempt to keep as many REG_EQUAL notes as
> possible as the notes can help later optimizations.  But even those
> equivalences are not necessarily safe.
>
> I'm pretty sure the right thing to do here is just drop the note regardless
> of whether or not its an invariant.
>
> jeff
>

[-- Attachment #2: pr62146.patch --]
[-- Type: text/x-patch, Size: 1888 bytes --]

Index: testsuite/g++.dg/opt/pr62146.C
===================================================================
--- testsuite/g++.dg/opt/pr62146.C	(revision 0)
+++ testsuite/g++.dg/opt/pr62146.C	(revision 0)
@@ -0,0 +1,51 @@
+/* PR rtl-optimization/62146 */
+/* { dg-do compile } */
+/* { dg-options "-O2 " } */
+class F
+{
+public:
+    virtual ~ F ();
+};
+template < class CL > class G:public F
+{
+    int *member_;
+public:
+    G ( int *b): member_ (0)
+    {
+    }
+};
+
+class D
+{
+public:
+    template < class CL > void RegisterNonTagCallback (int,
+            void (CL::
+                  *p3) ())
+    {
+        InternalRegisterNonTag (p3 ? new G < CL > ( 0) : 0);
+    } void InternalRegisterNonTag (F *);
+};
+
+void fn1 ();
+class C1
+{
+    void  foo();
+    class TokenType
+    {
+    public:
+        void AddToken ()
+        {
+        }
+    };
+    C1::TokenType bar_t;
+};
+D a;
+void C1::foo()
+{
+    if (&bar_t)
+        fn1 ();
+    for (int i = 0; i < sizeof 0; ++i)
+        a.RegisterNonTagCallback (0, &TokenType::AddToken);
+}
+
+/* { dg-final { scan-assembler-not "mov.*_ZN2C19TokenType8AddTokenEv, .\\\(" } } */
Index: ifcvt.c
===================================================================
--- ifcvt.c	(revision 214472)
+++ ifcvt.c	(working copy)
@@ -4387,17 +4387,14 @@ dead_or_predicable (basic_block test_bb, basic_blo
       insn = head;
       do
 	{
-	  rtx note, set;
+	  rtx note;
 
 	  if (! INSN_P (insn))
 	    continue;
 	  note = find_reg_note (insn, REG_EQUAL, NULL_RTX);
 	  if (! note)
 	    continue;
-	  set = single_set (insn);
-	  if (!set || !function_invariant_p (SET_SRC (set))
-	      || !function_invariant_p (XEXP (note, 0)))
-	    remove_note (insn, note);
+	  remove_note (insn, note);
 	} while (insn != end && (insn = NEXT_INSN (insn)));
 
       /* PR46315: when moving insns above a conditional branch, the REG_EQUAL

Re: [4.9] PR 62146

[Jeff Law]

On 09/04/14 19:27, Easwaran Raman wrote:
> I've attached the revised patch. Bootstrapped and no test regressions
> on x86_64/linux with 4.9 branch. Ok for 4.9 branch? While the bug
> doesn't show up in trunk, seems obvious that this should go to trunk
> as well. Is it ok for trunk if tests pass?
>
> Btw, is g++.dg/opt the right directory for the test to go?
>
> -----
>
> ChangeLog
>
> 2014-09-04  Easwaran Raman  <eraman@google.com>
>
>          PR rtl-optimization/62146
>          * ifcvt.c (dead_or_predicable): Make removal of REG_EQUAL note of
>          hoisted instruction unconditional.
>
> 2014-09-04  Easwaran Raman  <eraman@google.com>
>
>          PR rtl-optimization/62146
>          * testsuite/g++.dg/opt/pr62146.C: New.
OK for branch & trunk.

jeff