| Message ID | 5511862D.3010406@arm.com |
|---|---|
| State | New |
| Headers | show |
Hmmm. This is not the right fix: the tests Richard fixed, were failing because
of lack of constant propagation and DCE at compile-time, which then didn't
eliminate the call to link_error. The AArch64 test is failing because this from
aarch64/constraints.md:
(define_constraint "S"
"A constraint that matches an absolute symbolic address."
(and (match_code "const,symbol_ref,label_ref")
(match_test "aarch64_symbolic_address_p (op)")))
previously was seeing (and being satisfied by):
(const:DI (plus:DI (symbol_ref:DI ("test") [flags 0x3] <function_decl
0x7fb7c60300 test>)
(const_int 4 [0x4])))
but following Richard's patch the constraint is evaluated only on:
(reg/f:DI 73 [ D.2670 ])
--Alan
Alan Lawrence wrote:
> Following Richard Biener's patch at
> https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01064.html (r221532),
> gcc.target/aarch64/c-output-template-3.c fails with:
>
> c-output-template-3.c: In function 'test':
> c-output-template-3.c:7:5: error: impossible constraint in 'asm'
> __asm__ ("@ %c0" : : "S" (&test + 4));
>
> This patch fixes the test by changing options to -O in a similar manner to
> Richard's fixes to gcc.dg/pr15347.c and c-c++-common/pr19807-1.c.
>
> Ok for trunk?
>
> Cheers, Alan
>
> gcc/testsuite/ChangeLog:
>
> gcc.target/aarch64/c-output-template.c: Add -O, remove
> -Wno-pointer-arith.
On Tue, Mar 24, 2015 at 05:46:57PM +0000, Alan Lawrence wrote: > Hmmm. This is not the right fix: the tests Richard fixed, were failing because > of lack of constant propagation and DCE at compile-time, which then didn't > eliminate the call to link_error. The AArch64 test is failing because this from > aarch64/constraints.md: > > (define_constraint "S" > "A constraint that matches an absolute symbolic address." > (and (match_code "const,symbol_ref,label_ref") > (match_test "aarch64_symbolic_address_p (op)"))) > > previously was seeing (and being satisfied by): > > (const:DI (plus:DI (symbol_ref:DI ("test") [flags 0x3] <function_decl > 0x7fb7c60300 test>) > (const_int 4 [0x4]))) > > but following Richard's patch the constraint is evaluated only on: > > (reg/f:DI 73 [ D.2670 ]) I don't think we should get too concerned by this. There are a number of other constraints which we define which we can only satisfy given a level of optimisation. Take the I (immediate acceptable for an ADD instruction) constraint, which will fail for: int foo (int x) { int z = 5; __asm__ ("xxx %0 %1":"=r"(x) : "I"(z)); return x; } at O0 and happily produce: xxx x0 5 with optimisations. I think your original patch to add -O is just fine, but Marcus or Richard will need to approve it. Cheers, James
On Wed, Mar 25, 2015 at 06:27:49PM +0000, James Greenhalgh wrote: > I think your original patch to add -O is just fine, but Marcus or > Richard will need to approve it. I haven't seen any howls of objection from Marcus/Richard on this. As you say, your preferred fix for the "S" constraint is far too intrusive for GCC 5, but it seems sensible to investigate it for GCC 6. This implies that we do actually have some undesirable behaviour (albeit behaviour which we won't fix this year). So, we need a bugzilla ticket for the "S" constraint failing - and with it a testcase, which will probably look a lot like the one you are patching here! As for this testcase... The purpose is to check that the "%c" output modifier works properly with a complex label (see pr48637 for where this went wrong for the arm target), not to check the "S" constraint. As such, adding -O to cause the operand to be a symbol+offset again is a perfectly fine approach. In other words... > gcc/testsuite/ChangeLog: > > gcc.target/aarch64/c-output-template.c: Add -O, remove > -Wno-pointer-arith. > >diff --git a/gcc/testsuite/gcc.target/aarch64/c-output-template-3.c b/gcc/testsu >index c28837c..8bde4cb 100644 >--- a/gcc/testsuite/gcc.target/aarch64/c-output-template-3.c >+++ b/gcc/testsuite/gcc.target/aarch64/c-output-template-3.c >@@ -1,5 +1,5 @@ > /* { dg-do compile } */ >-/* { dg-options "-Wno-pointer-arith" } */ >+/* { dg-options "-O" } */ > > void > test (void) > This patch is OK. Cheers, James
Done - committed as r221905, and PR target/65689 filed on bugzilla. Cheers, Alan James Greenhalgh wrote: > On Wed, Mar 25, 2015 at 06:27:49PM +0000, James Greenhalgh wrote: >> I think your original patch to add -O is just fine, but Marcus or >> Richard will need to approve it. > > I haven't seen any howls of objection from Marcus/Richard on this. > > As you say, your preferred fix for the "S" constraint is far too intrusive > for GCC 5, but it seems sensible to investigate it for GCC 6. This implies > that we do actually have some undesirable behaviour (albeit behaviour > which we won't fix this year). So, we need a bugzilla ticket for the > "S" constraint failing - and with it a testcase, which will probably > look a lot like the one you are patching here! > > As for this testcase... The purpose is to check that the "%c" output > modifier works properly with a complex label (see pr48637 for where this > went wrong for the arm target), not to check the "S" constraint. As such, > adding -O to cause the operand to be a symbol+offset again is a perfectly > fine approach. > > In other words... > >> gcc/testsuite/ChangeLog: >> >> gcc.target/aarch64/c-output-template.c: Add -O, remove >> -Wno-pointer-arith. >> >> diff --git a/gcc/testsuite/gcc.target/aarch64/c-output-template-3.c b/gcc/testsu >> index c28837c..8bde4cb 100644 >> --- a/gcc/testsuite/gcc.target/aarch64/c-output-template-3.c >> +++ b/gcc/testsuite/gcc.target/aarch64/c-output-template-3.c >> @@ -1,5 +1,5 @@ >> /* { dg-do compile } */ >> -/* { dg-options "-Wno-pointer-arith" } */ >> +/* { dg-options "-O" } */ >> >> void >> test (void) >> > > This patch is OK. > > Cheers, > James >
diff --git a/gcc/testsuite/gcc.target/aarch64/c-output-template-3.c b/gcc/testsu index c28837c..8bde4cb 100644 --- a/gcc/testsuite/gcc.target/aarch64/c-output-template-3.c +++ b/gcc/testsuite/gcc.target/aarch64/c-output-template-3.c @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-Wno-pointer-arith" } */ +/* { dg-options "-O" } */ void test (void)
Following Richard Biener's patch at https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01064.html (r221532), gcc.target/aarch64/c-output-template-3.c fails with: c-output-template-3.c: In function 'test': c-output-template-3.c:7:5: error: impossible constraint in 'asm' __asm__ ("@ %c0" : : "S" (&test + 4)); This patch fixes the test by changing options to -O in a similar manner to Richard's fixes to gcc.dg/pr15347.c and c-c++-common/pr19807-1.c. Ok for trunk? Cheers, Alan gcc/testsuite/ChangeLog: gcc.target/aarch64/c-output-template.c: Add -O, remove -Wno-pointer-arith.