RFA; patch to fix PR90007

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?

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