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
