diff mbox series

[AArch64] Allow any offset for SVE addressing modes before reload

Message ID 20190211151653.GA12602@arm.com
State New
Headers show
Series [AArch64] Allow any offset for SVE addressing modes before reload | expand

Commit Message

Tamar Christina Feb. 11, 2019, 3:16 p.m. UTC 
Hi All,

On AArch64 aarch64_classify_address has a case for when it's non-strict
that will allow it to accept any byte offset from a reg when validating
an address in a given addressing mode.

This because reload would later make the address valid. SVE however requires
the address always be valid, but currently allows any address when a MEM +
offset is used.  This causes an ICE as nothing later forces the address to be
legitimate.

The patch forces aarch64_emit_sve_pred_move to ensure that the addressing mode
is valid for any loads/stores it creates, which follows the SVE way of handling
address classifications.

Bootstrapped on aarch64-none-linux-gnu and no issues.
Regtested on aarch64-none-elf with SVE on and no issues.

Ok for trunk?

Thanks,
Tamar

gcc/ChangeLog:

2019-02-11  Tamar Christina  <tamar.christina@arm.com>

	PR target/88847
	* config/aarch64/aarch64.c (aarch64_classify_address):
	For SVE enforce that the address is always valid when doing a MEM +
	offset.

gcc/testsuite/ChangeLog:

2019-02-11  Tamar Christina  <tamar.christina@arm.com>

	PR target/88847
	* gcc.target/aarch64/sve/pr88847.c: New test.

--

Comments

Richard Sandiford Feb. 11, 2019, 6:22 p.m. UTC | #1 
Tamar Christina <Tamar.Christina@arm.com> writes:
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 5df5a8b78439e69705e62845a4d1f86166a01894..59f03e688e58c1aab37629555c7b3f19e5075935 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -3414,6 +3414,14 @@ aarch64_expand_mov_immediate (rtx dest, rtx imm,
>  void
>  aarch64_emit_sve_pred_move (rtx dest, rtx pred, rtx src)
>  {
> +  /* Make sure that the address is legitimate.  */
> +  if (MEM_P (dest)
> +      && !aarch64_sve_struct_memory_operand_p (dest))
> +    {
> +      rtx addr = force_reg (Pmode, XEXP (dest, 0));
> +      dest = replace_equiv_address (dest, addr);
> +    }
> +
>    emit_insn (gen_rtx_SET (dest, gen_rtx_UNSPEC (GET_MODE (dest),
>  						gen_rtvec (2, pred, src),
>  						UNSPEC_MERGE_PTRUE)));

I think the same thing could happen for the src as well as dest.
The function is also used for single-vector modes, not just struct modes.

This code predated the support for "@" patterns.  Now that we have them,
it might be better to make:

(define_insn "*pred_mov<mode>"
  [(set (match_operand:SVE_ALL 0 "nonimmediate_operand" "=w, m")
	(unspec:SVE_ALL
	  [(match_operand:<VPRED> 1 "register_operand" "Upl, Upl")
	   (match_operand:SVE_ALL 2 "nonimmediate_operand" "m, w")]
	  UNSPEC_MERGE_PTRUE))]

and:

(define_insn_and_split "pred_mov<mode>"
  [(set (match_operand:SVE_STRUCT 0 "aarch64_sve_struct_nonimmediate_operand" "=w, Utx")
	(unspec:SVE_STRUCT
	  [(match_operand:<VPRED> 1 "register_operand" "Upl, Upl")
	   (match_operand:SVE_STRUCT 2 "aarch64_sve_struct_nonimmediate_operand" "Utx, w")]
	  UNSPEC_MERGE_PTRUE))]

public as "@aarch64_pred_mov<mode>" ("aarch64_" so that we don't
pollute the target-independent namespace).  Then we can use something like:

  expand_operand ops[3];
  create_output_operand (&ops[0], dest, mode);
  create_input_operand (&ops[1], pred, GET_MODE (pred));
  create_input_operand (&ops[2], src, mode);
  expand_insn (code_for_aarch64_pred_mov (mode), 3, ops);

(completely untested).

Thanks,
Richard
Tamar Christina Feb. 13, 2019, 10:32 a.m. UTC | #2 
Hi Richard,

The 02/11/2019 18:22, Richard Sandiford wrote:

> pollute the target-independent namespace).  Then we can use something like:
> 
>   expand_operand ops[3];
>   create_output_operand (&ops[0], dest, mode);
>   create_input_operand (&ops[1], pred, GET_MODE (pred));
>   create_input_operand (&ops[2], src, mode);
>   expand_insn (code_for_aarch64_pred_mov (mode), 3, ops);
> 
> (completely untested).
> 

Oh, I didn't know about these, thanks!

Bootstrapped on aarch64-none-linux-gnu and no issues.
Regtested on aarch64-none-elf with SVE on and no issues.

Ok for trunk?

Thanks,
Tamar

gcc/ChangeLog:

2019-02-13  Tamar Christina  <tamar.christina@arm.com>

	PR target/88847
	* config/aarch64/aarch64-sve.md (*pred_mov<mode>, pred_mov<mode>):
	Expose as @aarch64_pred_mov.
	* config/aarch64/aarch64.c (aarch64_classify_address):
	Use expand_insn which legitimizes operands.

gcc/testsuite/ChangeLog:

2019-02-13  Tamar Christina  <tamar.christina@arm.com>

	PR target/88847
	* gcc.target/aarch64/sve/pr88847.c: New test.

> Thanks,
> Richard

--
diff --git a/gcc/config/aarch64/aarch64-sve.md b/gcc/config/aarch64/aarch64-sve.md
index 5bb3422a7165452f1bb37cb4eb2bcd583189fee2..8bd62e55e69dd9adf26b9019ed1fc4ebfc750c59 100644
--- a/gcc/config/aarch64/aarch64-sve.md
+++ b/gcc/config/aarch64/aarch64-sve.md
@@ -170,7 +170,7 @@
 ;; all-true.  Note that this pattern is generated directly by
 ;; aarch64_emit_sve_pred_move, so changes to this pattern will
 ;; need changes there as well.
-(define_insn "*pred_mov<mode>"
+(define_insn "@aarch64_pred_mov<mode>"
   [(set (match_operand:SVE_ALL 0 "nonimmediate_operand" "=w, m")
 	(unspec:SVE_ALL
 	  [(match_operand:<VPRED> 1 "register_operand" "Upl, Upl")
@@ -400,7 +400,7 @@
 
 ;; Predicated structure moves.  This works for both endiannesses but in
 ;; practice is only useful for big-endian.
-(define_insn_and_split "pred_mov<mode>"
+(define_insn_and_split "@aarch64_pred_mov<mode>"
   [(set (match_operand:SVE_STRUCT 0 "aarch64_sve_struct_nonimmediate_operand" "=w, Utx")
 	(unspec:SVE_STRUCT
 	  [(match_operand:<VPRED> 1 "register_operand" "Upl, Upl")
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 5df5a8b78439e69705e62845a4d1f86166a01894..19a7cd372a1b7fe1b9210d42c24a520b48e6ae51 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3414,9 +3414,12 @@ aarch64_expand_mov_immediate (rtx dest, rtx imm,
 void
 aarch64_emit_sve_pred_move (rtx dest, rtx pred, rtx src)
 {
-  emit_insn (gen_rtx_SET (dest, gen_rtx_UNSPEC (GET_MODE (dest),
-						gen_rtvec (2, pred, src),
-						UNSPEC_MERGE_PTRUE)));
+  expand_operand ops[3];
+  machine_mode mode = GET_MODE (dest);
+  create_output_operand (&ops[0], dest, mode);
+  create_input_operand (&ops[1], pred, GET_MODE(pred));
+  create_input_operand (&ops[2], src, mode);
+  expand_insn (code_for_aarch64_pred_mov (mode), 3, ops);
 }
 
 /* Expand a pre-RA SVE data move from SRC to DEST in which at least one
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr88847.c b/gcc/testsuite/gcc.target/aarch64/sve/pr88847.c
new file mode 100644
index 0000000000000000000000000000000000000000..b7504add9a9f2eedf9421328a87b75b53d492860
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pr88847.c
@@ -0,0 +1,21 @@
+/* { dg-do assemble { target aarch64_asm_sve_ok } } */
+/* { dg-additional-options "-O0 -msve-vector-bits=256 -mbig-endian --save-temps" } */
+
+typedef struct _b {
+  __attribute__((__vector_size__(32))) int a[2];
+} b;
+
+b *c;
+
+void
+foo (void)
+{
+  char *p = '\0';
+  b e = c[0];
+}
+
+/* { dg-final { scan-assembler {\tld1w\tz[0-9]+.s, p[0-9]+/z, \[x[0-9]+\]\n} } } */
+/* { dg-final { scan-assembler {\tld1w\tz[0-9]+.s, p[0-9]+/z, \[x[0-9]+, #1, mul vl\]\n} } } */
+/* { dg-final { scan-assembler {\tst1w\tz[0-9]+.s, p[0-9]+, \[(sp|x[0-9]+)\]\n} } } */
+/* { dg-final { scan-assembler {\tst1w\tz[0-9]+.s, p[0-9]+, \[(sp|x[0-9]+), #1, mul vl\]\n} } } */
+
Richard Sandiford Feb. 13, 2019, 11:56 a.m. UTC | #3 
Tamar Christina <Tamar.Christina@arm.com> writes:
> Hi Richard,
>
> The 02/11/2019 18:22, Richard Sandiford wrote:
>> pollute the target-independent namespace).  Then we can use something like:
>> 
>>   expand_operand ops[3];
>>   create_output_operand (&ops[0], dest, mode);
>>   create_input_operand (&ops[1], pred, GET_MODE (pred));
>>   create_input_operand (&ops[2], src, mode);
>>   expand_insn (code_for_aarch64_pred_mov (mode), 3, ops);
>> 
>> (completely untested).
>> 
>
> Oh, I didn't know about these, thanks!
>
> Bootstrapped on aarch64-none-linux-gnu and no issues.
> Regtested on aarch64-none-elf with SVE on and no issues.
>
> Ok for trunk?

OK, thanks.

Richard

>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
>
> 	PR target/88847
> 	* config/aarch64/aarch64-sve.md (*pred_mov<mode>, pred_mov<mode>):
> 	Expose as @aarch64_pred_mov.
> 	* config/aarch64/aarch64.c (aarch64_classify_address):
> 	Use expand_insn which legitimizes operands.
>
> gcc/testsuite/ChangeLog:
>
> 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
>
> 	PR target/88847
> 	* gcc.target/aarch64/sve/pr88847.c: New test.
>
>> Thanks,
>> Richard
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 5df5a8b78439e69705e62845a4d1f86166a01894..59f03e688e58c1aab37629555c7b3f19e5075935 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3414,6 +3414,14 @@  aarch64_expand_mov_immediate (rtx dest, rtx imm,
 void
 aarch64_emit_sve_pred_move (rtx dest, rtx pred, rtx src)
 {
+  /* Make sure that the address is legitimate.  */
+  if (MEM_P (dest)
+      && !aarch64_sve_struct_memory_operand_p (dest))
+    {
+      rtx addr = force_reg (Pmode, XEXP (dest, 0));
+      dest = replace_equiv_address (dest, addr);
+    }
+
   emit_insn (gen_rtx_SET (dest, gen_rtx_UNSPEC (GET_MODE (dest),
 						gen_rtvec (2, pred, src),
 						UNSPEC_MERGE_PTRUE)));
diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr88847.c b/gcc/testsuite/gcc.target/aarch64/sve/pr88847.c
new file mode 100644
index 0000000000000000000000000000000000000000..b7504add9a9f2eedf9421328a87b75b53d492860
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pr88847.c
@@ -0,0 +1,21 @@ 
+/* { dg-do assemble { target aarch64_asm_sve_ok } } */
+/* { dg-additional-options "-O0 -msve-vector-bits=256 -mbig-endian --save-temps" } */
+
+typedef struct _b {
+  __attribute__((__vector_size__(32))) int a[2];
+} b;
+
+b *c;
+
+void
+foo (void)
+{
+  char *p = '\0';
+  b e = c[0];
+}
+
+/* { dg-final { scan-assembler {\tld1w\tz[0-9]+.s, p[0-9]+/z, \[x[0-9]+\]\n} } } */
+/* { dg-final { scan-assembler {\tld1w\tz[0-9]+.s, p[0-9]+/z, \[x[0-9]+, #1, mul vl\]\n} } } */
+/* { dg-final { scan-assembler {\tst1w\tz[0-9]+.s, p[0-9]+, \[(sp|x[0-9]+)\]\n} } } */
+/* { dg-final { scan-assembler {\tst1w\tz[0-9]+.s, p[0-9]+, \[(sp|x[0-9]+), #1, mul vl\]\n} } } */
+