| Message ID | 1515190466.18339.14.camel@cavium.com |
|---|---|
| State | New |
| Headers | show |
| Series | [aarch64,PR,target/83335] Fix regression, ICE on gcc.target/aarch64/asm-2.c | expand |
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
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.
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))); +}
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))); > +} >
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)