[Patch][aarch64][PR target/83335] Fix regression, ICE on gcc.target/aarch64/asm-2.c

[Patch][aarch64][PR target/83335] Fix regression, ICE on gcc.target/aarch64/asm-2.c

[Steve Ellcey]

This is a fix for PR target/83335.  We are asserting in
aarch64_print_address_internal because we have a non Pmode
address coming from an asm instruction.  My fix is to 
just allow this by checking this_is_asm_operands.  This is
what it was doing before the assert was added that caused
the ICE.

Verified that it fixed gcc.target/aarch64/asm-2.c in ILP32
mode and that it caused no regressions.

Steve Ellcey
sellcey@cavium.com


2018-01-05  Steve Ellcey  <sellcey@cavium.com>

	PR target/83335
	* config/aarch64/aarch64.c (aarch64_print_address_internal):
	Allow non Pmode address in asm statements.


diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index a189605..af74212 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -5670,8 +5670,9 @@ aarch64_print_address_internal (FILE *f, machine_mode mode, rtx x,
 {
   struct aarch64_address_info addr;
 
-  /* Check all addresses are Pmode - including ILP32.  */
-  gcc_assert (GET_MODE (x) == Pmode);
+  /* Check all addresses are Pmode - including ILP32,
+     unless this is coming from an asm statement.  */
+  gcc_assert (GET_MODE (x) == Pmode || this_is_asm_operands);
 
   if (aarch64_classify_address (&addr, x, mode, true, type))
     switch (addr.type)

AArch64 patch ping

[Jakub Jelinek]

Hi!

I'd like to ping this patch from Steve.

On Fri, Jan 05, 2018 at 02:14:26PM -0800, Steve Ellcey wrote:
> This is a fix for PR target/83335.  We are asserting in
> aarch64_print_address_internal because we have a non Pmode
> address coming from an asm instruction.  My fix is to 
> just allow this by checking this_is_asm_operands.  This is
> what it was doing before the assert was added that caused
> the ICE.
> 
> Verified that it fixed gcc.target/aarch64/asm-2.c in ILP32
> mode and that it caused no regressions.
> 
> Steve Ellcey
> sellcey@cavium.com
> 
> 
> 2018-01-05  Steve Ellcey  <sellcey@cavium.com>
> 
> 	PR target/83335
> 	* config/aarch64/aarch64.c (aarch64_print_address_internal):
> 	Allow non Pmode address in asm statements.
> 
> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index a189605..af74212 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -5670,8 +5670,9 @@ aarch64_print_address_internal (FILE *f, machine_mode mode, rtx x,
>  {
>    struct aarch64_address_info addr;
>  
> -  /* Check all addresses are Pmode - including ILP32.  */
> -  gcc_assert (GET_MODE (x) == Pmode);
> +  /* Check all addresses are Pmode - including ILP32,
> +     unless this is coming from an asm statement.  */
> +  gcc_assert (GET_MODE (x) == Pmode || this_is_asm_operands);
>  
>    if (aarch64_classify_address (&addr, x, mode, true, type))
>      switch (addr.type)

	Jakub

Re: [Patch][aarch64][PR target/83335] Fix regression, ICE on gcc.target/aarch64/asm-2.c

[Richard Earnshaw (lists)]

On 05/01/18 22:14, Steve Ellcey wrote:
> This is a fix for PR target/83335.  We are asserting in
> aarch64_print_address_internal because we have a non Pmode
> address coming from an asm instruction.  My fix is to 
> just allow this by checking this_is_asm_operands.  This is
> what it was doing before the assert was added that caused
> the ICE.
> 
> Verified that it fixed gcc.target/aarch64/asm-2.c in ILP32
> mode and that it caused no regressions.
> 
> Steve Ellcey
> sellcey@cavium.com
> 
> 
> 2018-01-05  Steve Ellcey  <sellcey@cavium.com>
> 
> 	PR target/83335
> 	* config/aarch64/aarch64.c (aarch64_print_address_internal):
> 	Allow non Pmode address in asm statements.
> 
> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index a189605..af74212 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -5670,8 +5670,9 @@ aarch64_print_address_internal (FILE *f, machine_mode mode, rtx x,
>  {
>    struct aarch64_address_info addr;
>  
> -  /* Check all addresses are Pmode - including ILP32.  */
> -  gcc_assert (GET_MODE (x) == Pmode);
> +  /* Check all addresses are Pmode - including ILP32,
> +     unless this is coming from an asm statement.  */
> +  gcc_assert (GET_MODE (x) == Pmode || this_is_asm_operands);
>  
>    if (aarch64_classify_address (&addr, x, mode, true, type))
>      switch (addr.type)
> 

Wouldn't it be better to call output_operand_lossage() with a suitable
diagnostic message?  If the operand isn't in Pmode assembly will
(should) fail anyway.

R.

Re: [Patch][aarch64][PR target/83335] Fix regression, ICE on gcc.target/aarch64/asm-2.c

[Steve Ellcey]

On Thu, 2018-02-15 at 14:01 +0000, Richard Earnshaw (lists) wrote:
> 
> Wouldn't it be better to call output_operand_lossage() with a suitable
> diagnostic message?  If the operand isn't in Pmode assembly will
> (should) fail anyway.
> 
> R.

How about this patch?  In addtion to the code change I updated asm-2.c
with the error message that you getin ILP32 mode and I added asm-4.c
which does not give an error message in either LP64 or ILP32 mode.

Steve Ellcey
sellcey@cavium.com

2018-02-16  Steve Ellcey  <sellcey@cavium.com>

	PR target/83335
	* config/aarch64/aarch64.c (aarch64_print_address_internal):
	Change gcc_assert call to output_operand_lossage.

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 7c9c6e5..34b75f8 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -7044,7 +7044,8 @@ aarch64_print_address_internal (FILE *f, machine_mode mode, rtx x,
   unsigned int size;
 
   /* Check all addresses are Pmode - including ILP32.  */
-  gcc_assert (GET_MODE (x) == Pmode);
+  if (GET_MODE (x) != Pmode)
+    output_operand_lossage ("invalid address mode");
 
   if (aarch64_classify_address (&addr, x, mode, true, type))
     switch (addr.type)


2018-02-16  Steve Ellcey  <sellcey@cavium.com>

	PR target/83335
	* gcc/testsuite/gcc.target/aarch64/asm-2.c: Add dg-error for
	ILP32 mode.
	* gcc/testsuite/gcc.target/aarch64/asm-4.c: New test.

diff --git a/gcc/testsuite/gcc.target/aarch64/asm-2.c b/gcc/testsuite/gcc.target/aarch64/asm-2.c
index 3f978f5..65b3a84 100644
--- a/gcc/testsuite/gcc.target/aarch64/asm-2.c
+++ b/gcc/testsuite/gcc.target/aarch64/asm-2.c
@@ -6,5 +6,5 @@ int x;
 void
 f (void)
 {
-  asm volatile ("%a0" :: "X" (&x));
+  asm volatile ("%a0" :: "X" (&x)); /* { dg-error "invalid address mode" "" { target ilp32 } } */
 }
diff --git a/gcc/testsuite/gcc.target/aarch64/asm-4.c b/gcc/testsuite/gcc.target/aarch64/asm-4.c
index e69de29..abe2af5 100644
--- a/gcc/testsuite/gcc.target/aarch64/asm-4.c
+++ b/gcc/testsuite/gcc.target/aarch64/asm-4.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O0" } */
+
+int x;
+
+void
+f (void)
+{
+  asm volatile ("%a0" :: "X" (__builtin_extend_pointer (&x)));
+}

Re: [Patch][aarch64][PR target/83335] Fix regression, ICE on gcc.target/aarch64/asm-2.c

[Richard Earnshaw (lists)]

On 17/02/18 00:04, Steve Ellcey wrote:
> On Thu, 2018-02-15 at 14:01 +0000, Richard Earnshaw (lists) wrote:
>>  
>> Wouldn't it be better to call output_operand_lossage() with a suitable
>> diagnostic message?  If the operand isn't in Pmode assembly will
>> (should) fail anyway.
>>
>> R.
> 
> How about this patch?  In addtion to the code change I updated asm-2.c
> with the error message that you getin ILP32 mode and I added asm-4.c
> which does not give an error message in either LP64 or ILP32 mode.
> 
> Steve Ellcey
> sellcey@cavium.com
> 
> 2018-02-16  Steve Ellcey  <sellcey@cavium.com>
> 
> 	PR target/83335
> 	* config/aarch64/aarch64.c (aarch64_print_address_internal):
> 	Change gcc_assert call to output_operand_lossage.

I think this is OK.  However, I note that __builtin_extend_pointer is
undocumented (it appears to be intended only for use in the exception
unwinding code).  Should it now be made an officially supported builtin?
 It appears to be the only semi-portable way of getting a Pmode address
out of a language level pointer and thus important for use on targets
where the two differ in size.

R.

> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 7c9c6e5..34b75f8 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -7044,7 +7044,8 @@ aarch64_print_address_internal (FILE *f, machine_mode mode, rtx x,
>    unsigned int size;
>  
>    /* Check all addresses are Pmode - including ILP32.  */
> -  gcc_assert (GET_MODE (x) == Pmode);
> +  if (GET_MODE (x) != Pmode)
> +    output_operand_lossage ("invalid address mode");
>  
>    if (aarch64_classify_address (&addr, x, mode, true, type))
>      switch (addr.type)
> 
> 
> 2018-02-16  Steve Ellcey  <sellcey@cavium.com>
> 
> 	PR target/83335
> 	* gcc/testsuite/gcc.target/aarch64/asm-2.c: Add dg-error for
> 	ILP32 mode.
> 	* gcc/testsuite/gcc.target/aarch64/asm-4.c: New test.
> 
> diff --git a/gcc/testsuite/gcc.target/aarch64/asm-2.c b/gcc/testsuite/gcc.target/aarch64/asm-2.c
> index 3f978f5..65b3a84 100644
> --- a/gcc/testsuite/gcc.target/aarch64/asm-2.c
> +++ b/gcc/testsuite/gcc.target/aarch64/asm-2.c
> @@ -6,5 +6,5 @@ int x;
>  void
>  f (void)
>  {
> -  asm volatile ("%a0" :: "X" (&x));
> +  asm volatile ("%a0" :: "X" (&x)); /* { dg-error "invalid address mode" "" { target ilp32 } } */
>  }
> diff --git a/gcc/testsuite/gcc.target/aarch64/asm-4.c b/gcc/testsuite/gcc.target/aarch64/asm-4.c
> index e69de29..abe2af5 100644
> --- a/gcc/testsuite/gcc.target/aarch64/asm-4.c
> +++ b/gcc/testsuite/gcc.target/aarch64/asm-4.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O0" } */
> +
> +int x;
> +
> +void
> +f (void)
> +{
> +  asm volatile ("%a0" :: "X" (__builtin_extend_pointer (&x)));
> +}
>