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

[Alan Lawrence <alan.lawrence@arm.com>]

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
>