PR target/46329: Reject Neon structure constants

PR target/46329: Reject Neon structure constants

[Richard Sandiford]

This patch fixed PR target/46329, which is a problem that occurs
when trying to reload:

    (set (reg:OI foo) (const_int 0))

There is no move alternative for moving constants directly into large
numbers of VFPs, and it probably isn't going to be sensible to do that
for all constants, even if proper support for large-int constants is
implemented in future.  It also isn't feasible to use the normal ARM
constant pool, since the vldN and vstN instructions don't support the
required addressing modes.

At the moment, reload instead tries to load the constant through GPRs,
having been told to do so by coproc_secondary_reload_class.
That doesn't work either because there aren't enough (consecutive)
GPRs to go around.

Instead, this patch forces constants to the target-independent
constant pool.  Even if we do optimise the handling for "easy"
constants like 0 in future, I think we'd still need this code
for the more complicated cases.

An alternative would have been to make preferred_reload_class
return NO_REGS, but we should get better code by exposing the
restriction earlier.

The patch applies on top of:

    http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00194.html
    http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00195.html

Tested on arm-linux-gnueabi.  OK to install?

Richard


gcc/
	PR target/46329
	* config/arm/arm.c (arm_legitimate_constant_p_1): Return false
	for all Neon struct constants.

gcc/testsuite/
2011-04-04  Richard Earnshaw  <rearnsha@arm.com>
	    Richard Sandiford  <richard.sandiford@linaro.org>

	PR target/46329
	* gcc.target/arm/pr46329.c: New test.

Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	2011-04-04 11:13:51.000000000 +0100
+++ gcc/config/arm/arm.c	2011-04-04 11:14:13.000000000 +0100
@@ -6566,8 +6566,14 @@ arm_tls_referenced_p (rtx x)
    When generating pic allow anything.  */
 
 static bool
-arm_legitimate_constant_p_1 (enum machine_mode mode ATTRIBUTE_UNUSED, rtx x)
+arm_legitimate_constant_p_1 (enum machine_mode mode, rtx x)
 {
+  /* At present, we have no support for Neon structure constants, so forbid
+     them here.  It might be possible to handle simple cases like 0 and -1
+     in future.  */
+  if (TARGET_NEON && VALID_NEON_STRUCT_MODE (mode))
+    return false;
+
   return flag_pic || !label_mentioned_p (x);
 }
 
Index: gcc/testsuite/gcc.target/arm/pr46329.c
===================================================================
--- /dev/null	2011-03-23 08:42:11.268792848 +0000
+++ gcc/testsuite/gcc.target/arm/pr46329.c	2011-04-04 11:14:18.000000000 +0100
@@ -0,0 +1,9 @@
+/* { dg-options "-O2" } */
+/* { dg-add-options arm_neon } */
+
+int __attribute__ ((vector_size (32))) x;
+void
+foo (void)
+{
+  x <<= x;
+}

Re: PR target/46329: Reject Neon structure constants

[Richard Earnshaw]

On Mon, 2011-04-04 at 11:34 +0100, Richard Sandiford wrote:
> This patch fixed PR target/46329, which is a problem that occurs
> when trying to reload:
> 
>     (set (reg:OI foo) (const_int 0))
> 
> There is no move alternative for moving constants directly into large
> numbers of VFPs, and it probably isn't going to be sensible to do that
> for all constants, even if proper support for large-int constants is
> implemented in future.  It also isn't feasible to use the normal ARM
> constant pool, since the vldN and vstN instructions don't support the
> required addressing modes.
> 
> At the moment, reload instead tries to load the constant through GPRs,
> having been told to do so by coproc_secondary_reload_class.
> That doesn't work either because there aren't enough (consecutive)
> GPRs to go around.
> 
> Instead, this patch forces constants to the target-independent
> constant pool.  Even if we do optimise the handling for "easy"
> constants like 0 in future, I think we'd still need this code
> for the more complicated cases.
> 
> An alternative would have been to make preferred_reload_class
> return NO_REGS, but we should get better code by exposing the
> restriction earlier.
> 
> The patch applies on top of:
> 
>     http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00194.html
>     http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00195.html
> 
> Tested on arm-linux-gnueabi.  OK to install?
> 
> Richard
> 

I'm uncomfortable about this.  Generally the ARM port doesn't work well
with the target-independent constant pool and it's better to assert that
this is empty when it comes to final assembly generation.  Can you
clarify by way of example how this patch is working please (ie some
sample output).

R.

Re: PR target/46329: Reject Neon structure constants

[Richard Sandiford]

Richard Earnshaw <rearnsha@arm.com> writes:
> I'm uncomfortable about this.  Generally the ARM port doesn't work well
> with the target-independent constant pool and it's better to assert that
> this is empty when it comes to final assembly generation.  Can you
> clarify by way of example how this patch is working please (ie some
> sample output).

Sure, for the testcase in the PR, we get:

        ldr     r8, .L3
        vldmia  r8, {d16-d19}
        ...
.L3:
        .word   .LC0
        ...
.LC0:
        .word   0
        [x 7]

(this is at -O0).  With -fPIC we get:

        ldr     r3, .L3
.LPIC0:
        add     r3, pc, r3
        vldmia  r3, {d16-d19}
        ...
.L3:
        .word   .LC0-(.LPIC0+8)

Richard

Re: PR target/46329: Reject Neon structure constants

[Richard Earnshaw]

On Mon, 2011-04-18 at 11:12 +0100, Richard Sandiford wrote:
> Richard Earnshaw <rearnsha@arm.com> writes:
> > I'm uncomfortable about this.  Generally the ARM port doesn't work well
> > with the target-independent constant pool and it's better to assert that
> > this is empty when it comes to final assembly generation.  Can you
> > clarify by way of example how this patch is working please (ie some
> > sample output).
> 
> Sure, for the testcase in the PR, we get:
> 
>         ldr     r8, .L3
>         vldmia  r8, {d16-d19}
>         ...
> .L3:
>         .word   .LC0
>         ...
> .LC0:
>         .word   0
>         [x 7]
> 
> (this is at -O0).  With -fPIC we get:
> 
>         ldr     r3, .L3
> .LPIC0:
>         add     r3, pc, r3
>         vldmia  r3, {d16-d19}
>         ...
> .L3:
>         .word   .LC0-(.LPIC0+8)
> 
> Richard
> 

I understand now.  I still don't particularly like this, but reworking
the minipools to allow really large constants isn't particularly
palatable either.  So OK.

R.