From patchwork Thu Mar 26 11:39:31 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alan Lawrence X-Patchwork-Id: 454966 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4D6CD1400A0 for ; Thu, 26 Mar 2015 22:39:45 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass reason="1024-bit key; unprotected key" header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=ogBh7yb/; dkim-adsp=none (unprotected policy); dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding; q=dns; s= default; b=KErUAStJ2CTZeFiwaBSQothah5k8UFlRtNyjxzD7t+jNsWgSSmiOf /stR/4L01XTNbtDRMfJVnCDeDZz49DZobgCtG8W0oG0ADwjh1cyhaHts9w3yWSyV C6f56AE4gnsnTEiu03yq42x6zZOQzr+av+IWgPcf3Y0zVT/uiGK1XE= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding; s=default; bh=YULkQTHA8segxpKGoOBHsoWeXWo=; b=ogBh7yb/pab6Df+8MvIIiS9p6Sfq oqEkcB8J/2oH0FSWj8A1WbVF5+mxqCKZ79FCKJV2e9Wh4OUovub/SriSGFeS+hBY IOB77jkiGOlY/6lOk9yt+5zZAiCIuWD2Dlgu96pNbmSXgciLzJQMTWwjw3FB5PT3 E9MtQap5qGAl2sc= Received: (qmail 74797 invoked by alias); 26 Mar 2015 11:39:38 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 74786 invoked by uid 89); 26 Mar 2015 11:39:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Received: from eu-smtp-delivery-143.mimecast.com (HELO eu-smtp-delivery-143.mimecast.com) (146.101.78.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 26 Mar 2015 11:39:36 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by uk-mta-13.uk.mimecast.lan; Thu, 26 Mar 2015 11:39:32 +0000 Received: from [10.2.207.65] ([10.1.2.79]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Thu, 26 Mar 2015 11:39:31 +0000 Message-ID: <5513EFF3.80805@arm.com> Date: Thu, 26 Mar 2015 11:39:31 +0000 From: Alan Lawrence User-Agent: Thunderbird 2.0.0.24 (X11/20101213) MIME-Version: 1.0 To: James Greenhalgh CC: "gcc-patches@gcc.gnu.org" , Marcus Shawcroft , Richard Biener Subject: Re: [PATCH][AArch64][Testsuite] Fix gcc.target/aarch64/c-output-template-3.c References: <5511862D.3010406@arm.com> <5511A311.2090702@arm.com> <20150325182749.GA28408@arm.com> In-Reply-To: <20150325182749.GA28408@arm.com> X-MC-Unique: 03f97uzARYSsqWTcTgicMA-1 X-IsSubscribed: yes 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] ) (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: 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] > 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 > 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;