RFA; patch to fix PR90007

RFA; patch to fix PR90007

[Vladimir Makarov]

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

The following patch fixes

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90007

Sometime ago a code which permits LRA to reload hard register into 
memory as it did for pseudo were added.  But this LRA possibility was 
not reflected in recog.c.  The following patch fixes this discrepancy 
and as a result fixes PR90007.

OK to commit?


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

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 278451)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2019-11-19  Vladimir Makarov  <vmakarov@redhat.com>
+
+	PR rtl-optimization/90007
+	* recog.c (constrain_operands): Permit hard registers too for
+	memory when LRA is used.
+
 2019-11-19  Martin Liska  <mliska@suse.cz>
 
 	* toplev.c (general_init): Move the call...
Index: recog.c
===================================================================
--- recog.c	(revision 278413)
+++ recog.c	(working copy)
@@ -2757,10 +2757,9 @@ constrain_operands (int strict, alternat
 			       /* Before reload, accept what reload can turn
 				  into a mem.  */
 			       || (strict < 0 && CONSTANT_P (op))
-			       /* Before reload, accept a pseudo,
+			       /* Before reload, accept a pseudo or hard register,
 				  since LRA can turn it into a mem.  */
-			       || (strict < 0 && targetm.lra_p () && REG_P (op)
-				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)
+			       || (strict < 0 && targetm.lra_p () && REG_P (op))
 			       /* During reload, accept a pseudo  */
 			       || (reload_in_progress && REG_P (op)
 				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)))
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog	(revision 278451)
+++ testsuite/ChangeLog	(working copy)
@@ -1,3 +1,8 @@
+2019-11-19  Vladimir Makarov  <vmakarov@redhat.com>
+
+	PR rtl-optimization/90007
+	* gcc.target/i386/pr90007.c: New test.
+
 2019-11-15  Andrew Sutton  <asutton@lock3software.com>
 
 	PR c++/89913
Index: testsuite/gcc.target/i386/pr90007.c
===================================================================
--- testsuite/gcc.target/i386/pr90007.c	(nonexistent)
+++ testsuite/gcc.target/i386/pr90007.c	(working copy)
@@ -0,0 +1,15 @@
+/* PR rtl-optimization/90007 */
+/* { dg-do compile { target x86_64-*-* } } */
+/* { dg-options "-march=bdver1 -mfpmath=387 -O1 -fschedule-insns -fselective-scheduling" } */
+
+void
+qj (int b9, int r9, int k4, int k0, int e7)
+{
+  (void) b9;
+  (void) r9;
+  (void) k4;
+
+  while (!!k0 == e7 * 1.1)
+    {
+    }
+}

Re: RFA; patch to fix PR90007

[Richard Biener]

On Tue, Nov 19, 2019 at 5:07 PM Vladimir Makarov <vmakarov@redhat.com> wrote:
>
> The following patch fixes
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90007
>
> Sometime ago a code which permits LRA to reload hard register into
> memory as it did for pseudo were added.  But this LRA possibility was
> not reflected in recog.c.  The following patch fixes this discrepancy
> and as a result fixes PR90007.
>
> OK to commit?

I guess the change is OK but for the bug itself it sounds like
selective scheduling doesn't properly recognize insns it
proagates into (and avoids doing that then)?  That is,
selective scheduling creates invalid RTL?

Thanks,
Richard.

>

Re: RFA; patch to fix PR90007

[Alexander Monakov]

On Wed, 20 Nov 2019, Richard Biener wrote:

> On Tue, Nov 19, 2019 at 5:07 PM Vladimir Makarov <vmakarov@redhat.com> wrote:
> >
> > The following patch fixes
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90007
> >
> > Sometime ago a code which permits LRA to reload hard register into
> > memory as it did for pseudo were added.  But this LRA possibility was
> > not reflected in recog.c.  The following patch fixes this discrepancy
> > and as a result fixes PR90007.
> >
> > OK to commit?
> 
> I guess the change is OK but for the bug itself it sounds like
> selective scheduling doesn't properly recognize insns it
> proagates into (and avoids doing that then)?  That is,
> selective scheduling creates invalid RTL?

We validate the substitution by invoking validate_replace_rtx_part_nosimplify
from substitute_reg_in_expr.  I think that should be sufficient?  I see similar
code in haifa-sched uses attempt_change, which also ultimately uses
apply_change_group.

FWIW, here's gdb session transcript showing that the substituted insn is
greenlighted:

Breakpoint 1, substitute_reg_in_expr (expr=0x248ee98, insn=0x7ffff791b880, undo=false) at /home/monoid/gcc-vecdoc/gcc/sel-sched.c:743
743       bool has_rhs = VINSN_RHS (*vi) != NULL;
(gdb) call debug_expr(expr)
[((17;r95=flt(r98);)type:set;count:3;)prio:8;orig_bb:2;]
(gdb) call debug_insn(insn)
([((35;r98=r8;)type:use;count:1;)prio:9;orig_bb:2;];seqno:2;)
(gdb) b validate_replace_rtx_part_nosimplify
Breakpoint 2 at 0xcc1cfb: file /home/monoid/gcc-vecdoc/gcc/recog.c, line 835.
(gdb) c
Continuing.

Breakpoint 2, validate_replace_rtx_part_nosimplify (from=from@entry=0x7ffff7aa2588, to=to@entry=0x7ffff7a9acd8, where=0x7ffff7aa2d00, insn=insn@entry=0x7ffff791b940) at /home/monoid/gcc-vecdoc/gcc/recog.c:835
835       validate_replace_rtx_1 (where, from, to, insn, false);
(gdb) call debug_rtx(from)
(reg:SI 98)
(gdb) call debug_rtx(to)
(reg:SI 36 r8 [ e7 ])
(gdb) call debug_rtx(*where)
(float:DF (reg:SI 98))
(gdb) call debug_rtx(insn)
(insn 39 0 0 (set (reg:DF 95)
        (float:DF (reg:SI 98))) 167 {*floatsidf2}
     (expr_list:REG_DEAD (reg:SI 98)
        (nil)))
(gdb) fin
Run till exit from #0  validate_replace_rtx_part_nosimplify (from=from@entry=0x7ffff7aa2588, to=to@entry=0x7ffff7a9acd8, where=0x7ffff7aa2d00, insn=insn@entry=0x7ffff791b940) at /home/monoid/gcc-vecdoc/gcc/recog.c:835
substitute_reg_in_expr (expr=0x248ee98, insn=<optimized out>, undo=<optimized out>) at /home/monoid/gcc-vecdoc/gcc/sel-sched.c:783
783           if (new_insn_valid)
Value returned is $1 = 1
(gdb) call debug_rtx(new_insn)
(insn 39 0 0 (set (reg:DF 95)
        (float:DF (reg:SI 36 r8 [ e7 ]))) 167 {*floatsidf2}
     (expr_list:REG_DEAD (reg:SI 98)
        (nil)))


Alexander

Re: RFA; patch to fix PR90007

[Vladimir Makarov]

On 2019-11-20 5:06 a.m., Richard Biener wrote:
> On Tue, Nov 19, 2019 at 5:07 PM Vladimir Makarov <vmakarov@redhat.com> wrote:
>> The following patch fixes
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90007
>>
>> Sometime ago a code which permits LRA to reload hard register into
>> memory as it did for pseudo were added.  But this LRA possibility was
>> not reflected in recog.c.  The following patch fixes this discrepancy
>> and as a result fixes PR90007.
>>
>> OK to commit?
> I guess the change is OK but for the bug itself it sounds like
> selective scheduling doesn't properly recognize insns it
> proagates into (and avoids doing that then)?  That is,
> selective scheduling creates invalid RTL?
>
It is hard for me to say what should be enough for new insn validation 
in any scheduler code.  I think recog code would be a safe choice as it 
is already used in DFA.

On the other hand using non-recog code for insn validation helped to 
find the code discrepancy between recog and LRA.

In any case I believe that the patch should be committed anyway and it 
fixes the problem.

Re: RFA; patch to fix PR90007

[Alexander Monakov]

On Wed, 20 Nov 2019, Alexander Monakov wrote:

> On Wed, 20 Nov 2019, Richard Biener wrote:
> 
> > On Tue, Nov 19, 2019 at 5:07 PM Vladimir Makarov <vmakarov@redhat.com> wrote:
> > >
> > > The following patch fixes
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90007
> > >
> > > Sometime ago a code which permits LRA to reload hard register into
> > > memory as it did for pseudo were added.  But this LRA possibility was
> > > not reflected in recog.c.  The following patch fixes this discrepancy
> > > and as a result fixes PR90007.
> > >
> > > OK to commit?
> > 
> > I guess the change is OK but for the bug itself it sounds like
> > selective scheduling doesn't properly recognize insns it
> > proagates into (and avoids doing that then)?  That is,
> > selective scheduling creates invalid RTL?
> 
> We validate the substitution by invoking validate_replace_rtx_part_nosimplify
> from substitute_reg_in_expr.  I think that should be sufficient?  I see similar
> code in haifa-sched uses attempt_change, which also ultimately uses
> apply_change_group.

Although looking at this more, I see that we specifically arrange for a call to
constrain_operands in replace_src_with_reg_ok_p (with a comment), but here in
substitute_reg_in_expr we have a '???' comment that seems to mention that
theoretically there might be a problem, but it never came up in practice.

So there's an inconsistency in sel-sched as well.

Alexander

Ping: RFA: patch to fix PR90007

[Vladimir Makarov]

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

This is the patch removing discrepancy between recog and LRA insn 
recognition:

https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01842.html


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

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 278451)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2019-11-19  Vladimir Makarov  <vmakarov@redhat.com>
+
+	PR rtl-optimization/90007
+	* recog.c (constrain_operands): Permit hard registers too for
+	memory when LRA is used.
+
 2019-11-19  Martin Liska  <mliska@suse.cz>
 
 	* toplev.c (general_init): Move the call...
Index: recog.c
===================================================================
--- recog.c	(revision 278413)
+++ recog.c	(working copy)
@@ -2757,10 +2757,9 @@ constrain_operands (int strict, alternat
 			       /* Before reload, accept what reload can turn
 				  into a mem.  */
 			       || (strict < 0 && CONSTANT_P (op))
-			       /* Before reload, accept a pseudo,
+			       /* Before reload, accept a pseudo or hard register,
 				  since LRA can turn it into a mem.  */
-			       || (strict < 0 && targetm.lra_p () && REG_P (op)
-				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)
+			       || (strict < 0 && targetm.lra_p () && REG_P (op))
 			       /* During reload, accept a pseudo  */
 			       || (reload_in_progress && REG_P (op)
 				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)))
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog	(revision 278451)
+++ testsuite/ChangeLog	(working copy)
@@ -1,3 +1,8 @@
+2019-11-19  Vladimir Makarov  <vmakarov@redhat.com>
+
+	PR rtl-optimization/90007
+	* gcc.target/i386/pr90007.c: New test.
+
 2019-11-15  Andrew Sutton  <asutton@lock3software.com>
 
 	PR c++/89913
Index: testsuite/gcc.target/i386/pr90007.c
===================================================================
--- testsuite/gcc.target/i386/pr90007.c	(nonexistent)
+++ testsuite/gcc.target/i386/pr90007.c	(working copy)
@@ -0,0 +1,15 @@
+/* PR rtl-optimization/90007 */
+/* { dg-do compile { target x86_64-*-* } } */
+/* { dg-options "-march=bdver1 -mfpmath=387 -O1 -fschedule-insns -fselective-scheduling" } */
+
+void
+qj (int b9, int r9, int k4, int k0, int e7)
+{
+  (void) b9;
+  (void) r9;
+  (void) k4;
+
+  while (!!k0 == e7 * 1.1)
+    {
+    }
+}

Re: Ping: RFA: patch to fix PR90007

[Richard Biener]

On Tue, Nov 26, 2019 at 3:57 PM Vladimir Makarov <vmakarov@redhat.com> wrote:
>
> This is the patch removing discrepancy between recog and LRA insn
> recognition:
>
> https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01842.html

OK.

Re: Ping: RFA: patch to fix PR90007

[Vladimir Makarov]

On 2019-11-27 3:31 a.m., Richard Biener wrote:
> On Tue, Nov 26, 2019 at 3:57 PM Vladimir Makarov <vmakarov@redhat.com> wrote:
>> This is the patch removing discrepancy between recog and LRA insn
>> recognition:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01842.html
> OK.
>
Thank you, Richard.  Committed as r278770.