Message ID | b85c0704-4e98-537b-9755-9b2b4146a966@redhat.com |
---|---|
State | New |
Headers | show |
Series | RFA; patch to fix PR90007 | expand |
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. >
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
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.
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
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) + { + } +}