[PATCH][AArch64][Testsuite] Fix gcc.target/aarch64/c-output-template-3.c

[PATCH][AArch64][Testsuite] Fix gcc.target/aarch64/c-output-template-3.c

[Alan Lawrence]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-c-out-3.patch --]
[-- Type: text/x-patch; name=fix-c-out-3.patch, Size: 374 bytes --]

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)

Re: [PATCH][AArch64][Testsuite] Fix gcc.target/aarch64/c-output-template-3.c

[Alan Lawrence]

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.

Re: [PATCH][AArch64][Testsuite] Fix gcc.target/aarch64/c-output-template-3.c

[James Greenhalgh]

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

Re: [PATCH][AArch64][Testsuite] Fix gcc.target/aarch64/c-output-template-3.c

[Alan Lawrence]

So I've dug into this a bit further, as follows.

Firstly, changing the test (without -O) to use an 'i' constraint, fixes the test 
(however, an "i" constraint is not correct for the instructions/asm where the 
"S" constraint is used, e.g. in the Linux kernel). This is because 
parse_input_constraint (in stmt.c) special-cases an 'i' constraint to set both 
allow_reg and allow_mem to false. expand_asm_operands (in cfgexpand.c) then 
passes EXPAND_INITIALIZER into expand_expr( a register ), which follows the 
definition of the register and returns

(const:DI (plus:DI (symbol_ref:DI ("test") [flags 0x3] <function_decl 
0x7fb7c60300 test>)
         (const_int 4 [0x4])))

which passes asm_operand_ok for an 'i' constraint (and indeed an 'S' constraint).

In contrast, when the test (without -O) has an 'S' constraint, 
parse_input_constraint falls back to:

	if (reg_class_for_constraint (cn) != NO_REGS
	    || insn_extra_address_constraint (cn))
	  *allows_reg = true;
	else if (insn_extra_memory_constraint (cn))
	  *allows_mem = true;
	else
	  {
	    /* Otherwise we can't assume anything about the nature of
	       the constraint except that it isn't purely registers.
	       Treat it like "g" and hope for the best.  */
	    *allows_reg = true;
	    *allows_mem = true;
	  }

which in our case sets allows_reg and allows_mem to true. This causes 
expand_asm_operands to pass EXPAND_NORMAL into expand_expr, which then just 
returns the
(reg/f:DI 73 [ D.2670 ])
passed in. This fails asm_operand_ok for the 'S' constraint (and indeed an 'i' 
constraint), leading to the error message on the test.

Thus, the following hack (which I do not propose!) to stmt.c fixes the testcase:

diff --git a/gcc/stmt.c b/gcc/stmt.c
index 45dc45f..d6515eb 100644
--- a/gcc/stmt.c
+++ b/gcc/stmt.c
@@ -400,7 +400,7 @@ parse_input_constraint (const char **constraint_p, int input
        case '?':  case '!':  case '*':  case '#':
        case '$':  case '^':
        case 'E':  case 'F':  case 'G':  case 'H':
-      case 's':  case 'i':  case 'n':
+      case 's':  case 'i':  case 'n':  case 'S':
        case 'I':  case 'J':  case 'K':  case 'L':  case 'M':
        case 'N':  case 'O':  case 'P':  case ',':
         break;

Clearly this is not an acceptable mechanism; we should have a generic method of 
defining constraints that accept both/neither registers+memory (e.g. we already 
have define_constraint, which currently accepts both except for those like 'i' 
which are special-cased in stmt.c; define_register_constraint which accepts 
registers only; and define_memory_constraint which accepts memory only).

However, I think this is too late in the development cycle for gcc5, and hence, 
I think the original testcase fix (dg-options "-O") is the best we can do for 
now (possibly unless we would prefer to XFAIL, but I think it's more valuable to 
make sure this works at -O).

The expansion of define_constraint, however, could be considered for gcc 6. 
Should I file a bugzilla ticket?

--Alan

James Greenhalgh wrote:
> 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
> 

Re: [PATCH][AArch64][Testsuite] Fix gcc.target/aarch64/c-output-template-3.c

[James Greenhalgh]

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

Re: [PATCH][AArch64][Testsuite] Fix gcc.target/aarch64/c-output-template-3.c

[Alan Lawrence]

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
>