[PATCH][AArch64] Fix ICE due to store_pair_lanes

[PATCH][AArch64] Fix ICE due to store_pair_lanes

[Wilco Dijkstra]

The recently added store_pair_lanes causes ICEs in output_operand.
This is due to aarch64_classify_address treating it like a 128-bit STR
rather than a STP. The valid immediate offsets don't fully overlap,
causing it to return false.  Eg. offset 264 is a valid 8-byte STP offset
but not a valid 16-byte STR offset since it isn't a multiple of 16.

The original instruction isn't passed in the printing code, so the context
is unclear.  The solution is to add a new operand formatting specifier
which is used for LDP/STP instructions like this.  This, like the Uml
constraint that applies to store_pair_lanes, uses PARALLEL when calling
aarch64_classify_address so that it knows it is an STP.
Also add the 'z' specifier for future use by load/store pair instructions.

Passes regress, OK for commit? 

ChangeLog:
2017-11-27  Wilco Dijkstra  <wdijkstr@arm.com>

	* config/aarch64/aarch64.c (aarch64_print_operand): Add new
	cases for printing LDP/STP memory addresses.
	(aarch64_print_address_internal): Renamed from
	aarch64_print_operand_address, added parameter, add Pmode check.
	(aarch64_print_ldpstp_address): New function for LDP/STP addresses.
	(aarch64_print_operand_address): Indirect to
	aarch64_print_address_internal.
	* config/aarch64/aarch64-simd.md (store_pair_lanes): Use new
	'y' operand output specifier.

--
diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
index cddd935d96589c52519334bd1b8c24e80ea475f3..b7dfbfaa1b00df113329e4b35397e2b6ce234786 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -3047,8 +3047,8 @@ (define_insn "store_pair_lanes<mode>"
 	   (match_operand:VDC 2 "register_operand" "w, r")))]
   "TARGET_SIMD"
   "@
-   stp\\t%d1, %d2, %0
-   stp\\t%x1, %x2, %0"
+   stp\\t%d1, %d2, %y0
+   stp\\t%x1, %x2, %y0"
   [(set_attr "type" "neon_stp, store_16")]
 )
 
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 60a8555b16296f07c68059bc54db803aeef0f369..e601f573bde4fae9a6e32af79ee987190895e088 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -150,6 +150,7 @@ static bool aarch64_builtin_support_vector_misalignment (machine_mode mode,
 							 bool is_packed);
 static machine_mode
 aarch64_simd_container_mode (scalar_mode mode, unsigned width);
+static void aarch64_print_ldpstp_address (FILE *f, machine_mode mode, rtx x);
 
 /* Major revision number of the ARM Architecture implemented by the target.  */
 unsigned aarch64_architecture_version;
@@ -5448,7 +5449,11 @@ static const int aarch64_nzcv_codes[] =
      'L':		Output constant address specified by X
 			with a relocation offset if appropriate.
      'G':		Prints address of X, specifying a PC relative
-			relocation mode if appropriate.  */
+			relocation mode if appropriate.
+     'y':		Output address of LDP or STP - this is used for
+			some LDP/STPs which don't use a PARALLEL in their
+			pattern (so the mode needs to be adjusted).
+     'z':		Output address of a typical LDP or STP.  */
 
 static void
 aarch64_print_operand (FILE *f, rtx x, int code)
@@ -5650,8 +5655,6 @@ aarch64_print_operand (FILE *f, rtx x, int code)
 
 	case MEM:
 	  output_address (GET_MODE (x), XEXP (x, 0));
-	  /* Check all memory references are Pmode - even with ILP32.  */
-	  gcc_assert (GET_MODE (XEXP (x, 0)) == Pmode);
 	  break;
 
 	case CONST:
@@ -5815,18 +5818,48 @@ aarch64_print_operand (FILE *f, rtx x, int code)
       }
       break;
 
+    case 'y':
+    case 'z':
+      {
+	machine_mode mode = GET_MODE (x);
+
+	if (GET_CODE (x) != MEM)
+	  {
+	    output_operand_lossage ("invalid operand for '%%%c'", code);
+	    return;
+	  }
+
+	if (code == 'y')
+	  {
+	    /* LDP/STP which uses a single double-width memory operand.
+	       Adjust the mode to appear like a typical LDP/STP.
+	       Currently this is supported for 16-byte accesses only.  */
+	    gcc_assert (GET_MODE_SIZE (mode) == 16);
+	    mode = DFmode;
+	  }
+
+	aarch64_print_ldpstp_address (f, mode, XEXP (x, 0));
+      }
+      break;
+
     default:
       output_operand_lossage ("invalid operand prefix '%%%c'", code);
       return;
     }
 }
 
+/* Print address 'x' of a memory access with mode 'mode'.
+   'op' is the context required by aarch64_classify_address.  It can either be
+   MEM for a normal memory access or PARALLEL for LDP/STP.  */
 static void
-aarch64_print_operand_address (FILE *f, machine_mode mode, rtx x)
+aarch64_print_address_internal (FILE *f, machine_mode mode, rtx x, RTX_CODE op)
 {
   struct aarch64_address_info addr;
 
-  if (aarch64_classify_address (&addr, x, mode, MEM, true))
+  /* Check all addresses are Pmode - including ILP32.  */
+  gcc_assert (GET_MODE (x) == Pmode);
+
+  if (aarch64_classify_address (&addr, x, mode, op, true))
     switch (addr.type)
       {
       case ADDRESS_REG_IMM:
@@ -5909,6 +5942,20 @@ aarch64_print_operand_address (FILE *f, machine_mode mode, rtx x)
   output_addr_const (f, x);
 }
 
+/* Print address 'x' of a LDP/STP with mode 'mode'.  */
+static void
+aarch64_print_ldpstp_address (FILE *f, machine_mode mode, rtx x)
+{
+  aarch64_print_address_internal (f, mode, x, PARALLEL);
+}
+
+/* Print address 'x' of a memory access with mode 'mode'.  */
+static void
+aarch64_print_operand_address (FILE *f, machine_mode mode, rtx x)
+{
+  aarch64_print_address_internal (f, mode, x, MEM);
+}
+
 bool
 aarch64_label_mentioned_p (rtx x)
 {

Re: [PATCH][AArch64] Fix ICE due to store_pair_lanes

[James Greenhalgh]

On Mon, Nov 27, 2017 at 03:20:29PM +0000, Wilco Dijkstra wrote:
> The recently added store_pair_lanes causes ICEs in output_operand.
> This is due to aarch64_classify_address treating it like a 128-bit STR
> rather than a STP. The valid immediate offsets don't fully overlap,
> causing it to return false.  Eg. offset 264 is a valid 8-byte STP offset
> but not a valid 16-byte STR offset since it isn't a multiple of 16.
> 
> The original instruction isn't passed in the printing code, so the context
> is unclear.  The solution is to add a new operand formatting specifier
> which is used for LDP/STP instructions like this.  This, like the Uml
> constraint that applies to store_pair_lanes, uses PARALLEL when calling
> aarch64_classify_address so that it knows it is an STP.
> Also add the 'z' specifier for future use by load/store pair instructions.
> 
> Passes regress, OK for commit? 

OK. But...

> +  if (aarch64_classify_address (&addr, x, mode, op, true))

This interface is not nice, resulting in...

> +/* Print address 'x' of a LDP/STP with mode 'mode'.  */
> +static void
> +aarch64_print_ldpstp_address (FILE *f, machine_mode mode, rtx x)
> +{
> +  aarch64_print_address_internal (f, mode, x, PARALLEL);
> +}
> +
> +/* Print address 'x' of a memory access with mode 'mode'.  */
> +static void
> +aarch64_print_operand_address (FILE *f, machine_mode mode, rtx x)
> +{
> +  aarch64_print_address_internal (f, mode, x, MEM);
> +}

These, which I *really* dislike.

Ideas on how to clean up this interface would be appreciated.

Thanks,
James

Re: [PATCH][AArch64] Fix ICE due to store_pair_lanes

[Christophe Lyon]

On 28 November 2017 at 19:15, James Greenhalgh <james.greenhalgh@arm.com> wrote:
> On Mon, Nov 27, 2017 at 03:20:29PM +0000, Wilco Dijkstra wrote:
>> The recently added store_pair_lanes causes ICEs in output_operand.
>> This is due to aarch64_classify_address treating it like a 128-bit STR
>> rather than a STP. The valid immediate offsets don't fully overlap,
>> causing it to return false.  Eg. offset 264 is a valid 8-byte STP offset
>> but not a valid 16-byte STR offset since it isn't a multiple of 16.
>>
>> The original instruction isn't passed in the printing code, so the context
>> is unclear.  The solution is to add a new operand formatting specifier
>> which is used for LDP/STP instructions like this.  This, like the Uml
>> constraint that applies to store_pair_lanes, uses PARALLEL when calling
>> aarch64_classify_address so that it knows it is an STP.
>> Also add the 'z' specifier for future use by load/store pair instructions.
>>
>> Passes regress, OK for commit?
>
> OK. But...
>
>> +  if (aarch64_classify_address (&addr, x, mode, op, true))
>
> This interface is not nice, resulting in...
>
>> +/* Print address 'x' of a LDP/STP with mode 'mode'.  */
>> +static void
>> +aarch64_print_ldpstp_address (FILE *f, machine_mode mode, rtx x)
>> +{
>> +  aarch64_print_address_internal (f, mode, x, PARALLEL);
>> +}
>> +
>> +/* Print address 'x' of a memory access with mode 'mode'.  */
>> +static void
>> +aarch64_print_operand_address (FILE *f, machine_mode mode, rtx x)
>> +{
>> +  aarch64_print_address_internal (f, mode, x, MEM);
>> +}
>
> These, which I *really* dislike.
>
> Ideas on how to clean up this interface would be appreciated.
>
> Thanks,
> James
>

Hi Wilco,

This breaks the build of aarch64-none-elf toolchains:

/tmp/8868058_9.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-aarch64_be-none-elf/gcc1/./gcc/xgcc
-B/tmp/8868058_9.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-aarch64_be-none-elf/gcc1/./gcc/
-B/tmp/8868058_9.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/tools/aarch64_be-none-elf/bin/
-B/tmp/8868058_9.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/tools/aarch64_be-none-elf/lib/
-isystem /tmp/8868058_9.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/tools/aarch64_be-none-elf/include
-isystem /tmp/8868058_9.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/tools/aarch64_be-none-elf/sys-include
   -g -O2 -mabi=ilp32 -O2  -g -O2 -DIN_GCC
-DCROSS_DIRECTORY_STRUCTURE  -W -Wall -Wwrite-strings -Wcast-qual
-Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition
-isystem ./include   -g -DIN_LIBGCC2 -fbuilding-libgcc
-fno-stack-protector -Dinhibit_libc  -I. -I. -I../../.././gcc
-I/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc
-I/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/.
-I/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/../gcc
-I/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/../include
   -o unwind-dw2-fde.o -MT unwind-dw2-fde.o -MD -MP -MF
unwind-dw2-fde.dep -fexceptions -c
/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2-fde.c
-fvisibility=hidden -DHIDE_EXPORTS
during RTL pass: final
/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2-fde.c:
In function 'search_object':
/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2-fde.c:1024:1:
internal compiler error: in aarch64_print_address_internal, at
config/aarch64/aarch64.c:5637
 }
 ^
0xf2259e aarch64_print_address_internal
/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/config/aarch64/aarch64.c:5637
0x7fb063 output_address(machine_mode, rtx_def*)
/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/final.c:3905
0x7fe563 output_asm_insn(char const*, rtx_def**)
/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/final.c:3766
0x7fed04 final_scan_insn(rtx_insn*, _IO_FILE*, int, int, int*)
/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/final.c:3064
0x7ffcfa final(rtx_insn*, _IO_FILE*, int)
/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/final.c:2052
0x80088b rest_of_handle_final
/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/final.c:4490
0x80088b execute
/tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/final.c:4564
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
make[4]: *** [unwind-dw2-fde.o] Error 1
make[4]: Leaving directory
`/tmp/8868058_9.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-aarch64_be-none-elf/gcc1/aarch64_be-none-elf/ilp32/libgcc'
make[3]: *** [multi-do] Error 1


Can you have a look?

Thanks,

Christophe

Re: [PATCH][AArch64] Fix ICE due to store_pair_lanes

[Steve Ellcey]

On Wed, 2017-11-29 at 21:13 +0100, Christophe Lyon wrote:

> Hi Wilco,
> 
> This breaks the build of aarch64-none-elf toolchains:
> 
> /tmp/8868058_9.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/final.c:4564
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See <https://gcc.gnu.org/bugs/> for instructions.
> make[4]: *** [unwind-dw2-fde.o] Error 1
> make[4]: Leaving directory
> `/tmp/8868058_9.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-aarch64_be-none-elf/gcc1/aarch64_be-none-elf/ilp32/libgcc'
> make[3]: *** [multi-do] Error 1

I am seeing this failure too, on my aarch64-linux-gnu build where I
have ILP32 enabled.

Steve Ellcey
sellcey@cavium.com

Re: [PATCH][AArch64] Fix ICE due to store_pair_lanes

[Steve Ellcey]

FYI: Here is a cut down test case showing the failure:

int foo (void) { }
extern void plunk ();
int splat (void)
{
      static int once = 0;
      plunk (&once, foo);
}

% obj/gcc/gcc/cc1 -mabi=ilp32 -O2 -quiet x.i

during RTL pass: final
x.i: In function ‘splat’:
x.i:7:1: internal compiler error: in aarch64_print_address_internal, at config/aarch64/aarch64.c:5638
 }
 ^
0x14286c7 aarch64_print_address_internal
	/home/sellcey/gcc-spec-
ilp32/src/gcc/gcc/config/aarch64/aarch64.c:5638
0x1428d4b aarch64_print_operand_address
	/home/sellcey/gcc-spec-
ilp32/src/gcc/gcc/config/aarch64/aarch64.c:5735
0xb5fa93 output_address(machine_mode, rtx_def*)
	/home/sellcey/gcc-spec-ilp32/src/gcc/gcc/final.c:3905
0xb5f40b output_asm_insn(char const*, rtx_def**)
	/home/sellcey/gcc-spec-ilp32/src/gcc/gcc/final.c:3766
0xb5e0df final_scan_insn(rtx_insn*, _IO_FILE*, int, int, int*)
	/home/sellcey/gcc-spec-ilp32/src/gcc/gcc/final.c:3064
0xb5bfbb final(rtx_insn*, _IO_FILE*, int)
	/home/sellcey/gcc-spec-ilp32/src/gcc/gcc/final.c:2052
0xb60d27 rest_of_handle_final
	/home/sellcey/gcc-spec-ilp32/src/gcc/gcc/final.c:4490
0xb6110f execute
	/home/sellcey/gcc-spec-ilp32/src/gcc/gcc/final.c:4564
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.