Re: [PATCH] combine: Do not combine moves from hard registers

[Renlin Li <renlin dot li at foss dot arm dot com>]

Hi Segher,

On 11/03/2018 02:34 AM, Jeff Law wrote:
> On 11/2/18 5:54 PM, Segher Boessenkool wrote:
> > On Fri, Nov 02, 2018 at 06:03:20PM -0500, Segher Boessenkool wrote:
> > > > The original rtx is generated by expand_builtin_setjmp_receiver to adjust
> > > > the frame pointer.
> > > >
> > > > And later in LRA, it will try to eliminate frame_pointer with hard frame
> > > > pointer which is
> > > > defined the ELIMINABLE_REGS.
> > > >
> > > > Your change split the insn into two.
> > > > This makes it doesn't match the "from" and "to" regs defined in
> > > > ELIMINABLE_REGS.
> > > > The if statement to generate the adjustment insn is been skipt.
> > > > And the original instruction is just been deleted!
> > > I don't follow why, or what should have prevented it from being deleted.
> > >
> > > > Probably, we don't want to split the move rtx if they are related to
> > > > entries defined in ELIMINABLE_REGS?
> > > One thing I can easily do is not making an intermediate pseudo when copying
> > > *to* a fixed reg, which sfp is.  Let me try if that helps the testcase I'm
> > > looking at (setjmp-4.c).
> > This indeed helps, see patch below.  Could you try that on the whole
> > testsuite?
> >
> > Thanks,
> >
> >
> > Segher
> >
> >
> > p.s. It still is a problem in the arm backend, but this won't hurt combine,
> > so why not.
> >
> >
> >  From 814ca23ce05384d017b3c2bff41ab61cf5446e46 Mon Sep 17 00:00:00 2001
> > Message-Id: <814ca23ce05384d017b3c2bff41ab61cf5446e46.1541202704.git.segher@kernel.crashing.org>
> > From: Segher Boessenkool <segher@kernel.crashing.org>
> > Date: Fri, 2 Nov 2018 23:33:32 +0000
> > Subject: [PATCH] combine: Don't break up copy from hard to fixed reg
> >
> > ---
> >   gcc/combine.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/gcc/combine.c b/gcc/combine.c
> > index dfb0b44..15e941a 100644
> > --- a/gcc/combine.c
> > +++ b/gcc/combine.c
> > @@ -14998,6 +14998,8 @@ make_more_copies (void)
> >   	    continue;
> >   	  if (TEST_HARD_REG_BIT (fixed_reg_set, REGNO (src)))
> >   	    continue;
> > +	  if (REG_P (dest) && TEST_HARD_REG_BIT (fixed_reg_set, REGNO (dest)))
> > +	    continue;
> >   
> >   	  rtx new_reg = gen_reg_rtx (GET_MODE (dest));
> >   	  rtx_insn *new_insn = gen_move_insn (new_reg, src);
> > -- 1.8.3.1
> It certainly helps the armeb test results.

Yes, I can also see it helps a lot with the regression test.
Thanks for working on it!


Beside the correctness issue, there are performance regression issues as other people also reported.

I analysised a case, which is gcc.c-torture/execute/builtins/memcpy-chk.c
In this case, two additional register moves and callee saves are emitted.

The problem is that, make_more_moves split a move into two. Ideally, the RA could figure out and
make the best register allocation. However, in reality, scheduler in some cases will reschedule
the instructions, and which changes the live-range of registers. And thus change the interference graph
of pseudo registers.

This will force the RA to choose a different register for it, and make the move instruction not redundant,
at least, not possible for RA to eliminate it.

For example,

set r102, r1

After combine:
insn x: set r103, r1
insn x+1: set r22, r103

After scheduler:
insn x: set r103, r1
...
...
...
insn x+1: set r102, r103

After IRA, r1 could be assigned to operands used in instructions in between insn x and x+1.
so r23 is conflicting with r1. LRA has to assign r23 a different hard register.
This cause one additional move, and probably one more callee save/restore.

Nothing is obviously wrong here. But...

One simple case probably not beneficial is to split hard register store.
According to your comment on make_more_moves, you might want to apply the transformation only
on hard-reg-to-pseudo-copy?

Regards,
Renlin




> Jeff