This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]