Re: [Patch, lra] PR70751, correct the cost for spilling non-pseudo into memory

[Jiong Wang <jiong dot wang at foss dot arm dot com>]

On 28/06/16 11:01, Jiong Wang wrote:
> On 28/06/16 10:51, Jiong Wang wrote:
> > On 27/06/16 17:26, Dominik Vogt wrote:
> > > On Thu, Jun 09, 2016 at 09:52:39AM -0600, Jeff Law wrote:
> > > > On 06/08/2016 10:47 AM, Jiong Wang wrote:
> > > > > As discussed on the PR
> > > > >
> > > > >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70751,
> > > > >
> > > > > here is the patch.
> > > This commit has introduced an ICE with s390x, march=z13.  Is it a
> > > backend bug or one in the middleend?
> > >
> > > -- x.c --
> > > void foo(int *n, int off, int *m)
> > > {
> > >    int i;
> > >
> > >    for(i = 0 ;i <= 3; i++)
> > >      {
> > >        n[off + i] = m[2 * i];
> > >        n[off + 7 - i] = m[2 * i + 1];
> > >      }
> > > }
> > > -- snip --
> > >
> > >    $ gcc -S -O3 -march=z13 x.c
> > > x.c: In function âfooâ:
> > > x.c:10:1: internal compiler error: Max. number of generated reload 
> > > insns per insn is achieved (90)
> >
> > I tried the following on gcc trunk (r237817, 28-June-2016), can't 
> > reproduce the issue.
> >
> > configure --target=s390-linux
> > ./cc1 -O3 1.c -nostdinc -march=z13
> >
> > Can you please paste "gcc -v -S -O3 -march=z13 x.c" to see what's 
> > passed to cc1?
>
> Reproduced with --target=s390x-linux.

The insn causing trouble is

(insn 41 38 120 4 (set (mem:V4SI (plus:DI (plus:DI (reg/v/f:DI 116 [ n ])
                    (reg:DI 69 [ _39 ]))
                (const_int -16 [0xfffffffffffffff0]))
        (subreg:V4SI (reg:V16QI 134) 0)) 1.c:8 300 {movv4si}

The mem address actually doesn't qualify s390_short_displacement as the
disp is negative number, but s390 has allowed it to pass
legitiate_address_p check during early address generation stage. The
following draft patch fixed this ICE.

(plus:DI (plus:DI (reg/v/f:DI 116 [ n ])
                    (reg:DI 69 [ _39 ]))

So my first impression is TARGET_LEGITIMATE_ADDRESS_P on s390 do need a
fix here. The following draft patch fix this, my fix may be in
correct as normally we will allow illegal constant offset if it's
associated with virtual frame register before virtual register
elimination, I don't know if that should be considered here. Also I
don't know if such check should be constrainted under some architecture
features.

--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -4374,6 +4374,11 @@ s390_legitimate_address_p (machine_mode mode, rtx 
addr, bool strict)
               || REGNO_REG_CLASS (REGNO (ad.indx)) == ADDR_REGS))
          return false;
     }
+
+  if (ad.disp
+      && !s390_short_displacement (ad.disp))
+    return false;
+
   return true;
 }

Meanwhile I feel LRA might be too strict here, as we can't assume all
address legitimized before lra, right? we still need to handle reload
illegal address. While now, a reload of

  set (mem (reg + reg + offset)) regA

  (The offset is out of range that the inner address doesn't pass
   constraint check.)

into

   regB <- reg + reg + offset
  (set (mem (regB)) regA)

is treate as spill after r237277, so I feel the following check in
lra-constraints.c also needs a relax?

if (no_regs_p && !(REG_P (op) && hard_regno[nop] < 0))


Comments?