Re: [PATCH][AArch64] Add TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Tue, Dec 15, 2015 at 10:54:49AM +0000, Wilco Dijkstra wrote:
> ping
> 
> > -----Original Message-----
> > From: Wilco Dijkstra [mailto:Wilco.Dijkstra@arm.com]
> > Sent: 06 November 2015 20:06
> > To: 'gcc-patches@gcc.gnu.org'
> > Subject: [PATCH][AArch64] Add TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
> > 
> > This patch adds support for the TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
> > hook. When the cost of GENERAL_REGS and FP_REGS is identical, the register
> > allocator always uses ALL_REGS even when it has a much higher cost. The
> > hook changes the class to either FP_REGS or GENERAL_REGS depending on the
> > mode of the register. This results in better register allocation overall,
> > fewer spills and reduced codesize - particularly in SPEC2006 gamess.
> > 
> > GCC regression passes with several minor fixes.
> > 
> > OK for commit?
> > 
> > ChangeLog:
> > 2015-11-06  Wilco Dijkstra  <wdijkstr@arm.com>
> > 
> > 	* gcc/config/aarch64/aarch64.c
> > 	(TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS): New define.
> > 	(aarch64_ira_change_pseudo_allocno_class): New function.
> > 	* gcc/testsuite/gcc.target/aarch64/cvtf_1.c: Build with -O2.
> > 	* gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c
> > 	(test_corners_sisd_di): Improve force to SIMD register.
> > 	(test_corners_sisd_si): Likewise.
> > 	* gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c: Build with -O2.
> > 	* gcc/testsuite/gcc.target/aarch64/vect-ld1r-compile-fp.c:
> > 	Remove scan-assembler check for ldr.

Drop the gcc/ from the ChangeLog.

> > --
> >  gcc/config/aarch64/aarch64.c                       | 22 ++++++++++++++++++++++
> >  gcc/testsuite/gcc.target/aarch64/cvtf_1.c          |  2 +-
> >  gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c  |  4 ++--
> >  gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c     |  2 +-
> >  .../gcc.target/aarch64/vect-ld1r-compile-fp.c      |  1 -

These testsuite changes concern me a bit, and you don't mention them beyond
saying they are minor fixes...

> > diff --git a/gcc/testsuite/gcc.target/aarch64/cvtf_1.c b/gcc/testsuite/gcc.target/aarch64/cvtf_1.c
> > index 5f2ff81..96501db 100644
> > --- a/gcc/testsuite/gcc.target/aarch64/cvtf_1.c
> > +++ b/gcc/testsuite/gcc.target/aarch64/cvtf_1.c
> > @@ -1,5 +1,5 @@
> >  /* { dg-do run } */
> > -/* { dg-options "-save-temps -fno-inline -O1" } */
> > +/* { dg-options "-save-temps -fno-inline -O2" } */

This one says we have a code-gen regression at -O1 ?

> > 
> >  #define FCVTDEF(ftype,itype) \
> >  void \
> > diff --git a/gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c b/gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c
> > index 363f554..8465c89 100644
> > --- a/gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c
> > +++ b/gcc/testsuite/gcc.target/aarch64/scalar_shift_1.c
> > @@ -186,9 +186,9 @@ test_corners_sisd_di (Int64x1 b)
> >  {
> >    force_simd_di (b);
> >    b = b >> 63;
> > +  force_simd_di (b);
> >    b = b >> 0;
> >    b += b >> 65; /* { dg-warning "right shift count >= width of type" } */
> > -  force_simd_di (b);

This one I don't understand, but seems to say that we've decided to move
b out of FP_REGS after getting it in there for b = b << 63; ? So this is
another register allocator regression?

> > diff --git a/gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c b/gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c
> > index a49db3e..c5a9c52 100644
> > --- a/gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c
> > +++ b/gcc/testsuite/gcc.target/aarch64/vdup_lane_2.c
> > @@ -1,6 +1,6 @@
> >  /* Test vdup_lane intrinsics work correctly.  */
> >  /* { dg-do run } */
> > -/* { dg-options "-O1 --save-temps" } */
> > +/* { dg-options "-O2 --save-temps" } */

Another -O1 regression ?

> > 
> >  #include <arm_neon.h>
> > 
> > diff --git a/gcc/testsuite/gcc.target/aarch64/vect-ld1r-compile-fp.c b/gcc/testsuite/gcc.target/aarch64/vect-ld1r-compile-fp.c
> > index 66e0168..4711c61 100644
> > --- a/gcc/testsuite/gcc.target/aarch64/vect-ld1r-compile-fp.c
> > +++ b/gcc/testsuite/gcc.target/aarch64/vect-ld1r-compile-fp.c
> > @@ -8,6 +8,5 @@ DEF (float)
> >  DEF (double)
> > 
> >  /* { dg-final { scan-assembler "ld1r\\t\{v\[0-9\]+\.4s"} } */
> > -/* { dg-final { scan-assembler "ldr\\t\x\[0-9\]+"} } */
> >  /* { dg-final { scan-assembler "ld1r\\t\{v\[0-9\]+\.2d"} } */

This one is fine, I don't really understand what it was hoping to catch
in the first place!

Could you go in to some detail about why your testsuite changes are correct?

Thanks,
James