From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32531 invoked by alias); 19 Sep 2014 10:40:29 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 32519 invoked by uid 89); 19 Sep 2014 10:40:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: cam-smtp0.cambridge.arm.com Received: from fw-tnat.cambridge.arm.com (HELO cam-smtp0.cambridge.arm.com) (217.140.96.21) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 19 Sep 2014 10:40:25 +0000 Received: from arm.com (e106375-lin.cambridge.arm.com [10.1.203.160]) by cam-smtp0.cambridge.arm.com (8.13.8/8.13.8) with ESMTP id s8JAeMpd012991; Fri, 19 Sep 2014 11:40:22 +0100 Date: Fri, 19 Sep 2014 10:40:00 -0000 From: James Greenhalgh To: Jeff Law Cc: "gcc-patches@gcc.gnu.org" Subject: Re: [Patch] Teach genrecog/genoutput that scratch registers require write constraint modifiers Message-ID: <20140919104021.GA17879@arm.com> References: <1411035561-17445-1-git-send-email-james.greenhalgh@arm.com> <541B4487.40309@redhat.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="T4sUOijqQbZv57TR" Content-Disposition: inline In-Reply-To: <541B4487.40309@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg01615.txt.bz2 --T4sUOijqQbZv57TR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 1599 On Thu, Sep 18, 2014 at 09:45:59PM +0100, Jeff Law wrote: > On 09/18/14 04:19, James Greenhalgh wrote: > > > > Hi, > > > > As discussed in https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01334.html > > The construct > > > > (clobber (match_scratch 0 "r")) > > > > is invalid - operand 0 must be marked either write or read/write. > > > > Likewise > > > > (match_* 0 "&r") > > > > is invalid, marking an operand earlyclobber does not remove the need to > > also mark it write or read/write. > > > > This patch adds checking for these two error conditions to the generator > > programs and documents the restriction. > > > > Bootstrapped on x86, ARM and AArch64 with no new issues. > > > > Ok? > > > > Thanks, > > James > > > > --- > > 2014-09-17 James Greenhalgh > > > > * doc/md.texi (Modifiers): Consistently use "read/write" > > nomenclature rather than "input/output". > > * genrecog.c (constraints_supported_in_insn_p): New. > > (validate_pattern): If needed, also check constraints on > > MATCH_SCRATCH operands. > > * genoutput.c (validate_insn_alternatives): Catch earlyclobber > > operands with no '=' or '+' modifier. > > + if (c == '=' || c == '+') > + seen_inout = true; > > Isn't "seen_input" poorly named here? ISTM what we're checking for is > if we've seen an operand that will be written to. > > Doesn't it make sense to use read/write nomenclature in the comments and > variable names too? > > So with those nits fixed, this patch is OK. Thanks Jeff, In the end I committed the attached as revision 215388. Cheers, James --T4sUOijqQbZv57TR Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-Patch-Teach-genrecog-genoutput-that-scratch-register.patch" Content-length: 7842 Index: gcc/ChangeLog =================================================================== --- gcc/ChangeLog (revision 215387) +++ gcc/ChangeLog (working copy) @@ -1,5 +1,15 @@ 2014-09-19 James Greenhalgh + * doc/md.texi (Modifiers): Consistently use "read/write" + nomenclature rather than "input/output". + * genrecog.c (constraints_supported_in_insn_p): New. + (validate_pattern): If needed, also check constraints on + MATCH_SCRATCH operands. + * genoutput.c (validate_insn_alternatives): Catch earlyclobber + operands with no '=' or '+' modifier. + +2014-09-19 James Greenhalgh + * config/aarch64/aarch64.md (stack_protect_test_): Mark scratch register as written. Index: gcc/genoutput.c =================================================================== --- gcc/genoutput.c (revision 215387) +++ gcc/genoutput.c (working copy) @@ -769,6 +769,7 @@ char c; int which_alternative = 0; int alternative_count_unsure = 0; + bool seen_write = false; for (p = d->operand[start].constraint; (c = *p); p += len) { @@ -777,6 +778,18 @@ error_with_line (d->lineno, "character '%c' can only be used at the" " beginning of a constraint string", c); + + if (c == '=' || c == '+') + seen_write = true; + + /* Earlyclobber operands must always be marked write-only + or read/write. */ + if (!seen_write && c == '&') + error_with_line (d->lineno, + "earlyclobber operands may not be" + " read-only in alternative %d", + which_alternative); + if (ISSPACE (c) || strchr (indep_constraints, c)) len = 1; else if (ISDIGIT (c)) Index: gcc/genrecog.c =================================================================== --- gcc/genrecog.c (revision 215387) +++ gcc/genrecog.c (working copy) @@ -415,7 +415,19 @@ return NULL; } +/* In DEFINE_EXPAND, DEFINE_SPLIT, and DEFINE_PEEPHOLE2, we + don't use the MATCH_OPERAND constraint, only the predicate. + This is confusing to folks doing new ports, so help them + not make the mistake. */ +static bool +constraints_supported_in_insn_p (rtx insn) +{ + return !(GET_CODE (insn) == DEFINE_EXPAND + || GET_CODE (insn) == DEFINE_SPLIT + || GET_CODE (insn) == DEFINE_PEEPHOLE2); +} + /* Check for various errors in patterns. SET is nonnull for a destination, and is the complete set pattern. SET_CODE is '=' for normal sets, and '+' within a context that requires in-out constraints. */ @@ -432,7 +444,32 @@ switch (code) { case MATCH_SCRATCH: - return; + { + const char constraints0 = XSTR (pattern, 1)[0]; + + if (!constraints_supported_in_insn_p (insn)) + { + if (constraints0) + { + error_with_line (pattern_lineno, + "constraints not supported in %s", + rtx_name[GET_CODE (insn)]); + } + return; + } + + /* If a MATCH_SCRATCH is used in a context requiring an write-only + or read/write register, validate that. */ + if (set_code == '=' + && constraints0 != '=' + && constraints0 != '+') + { + error_with_line (pattern_lineno, + "operand %d missing output reload", + XINT (pattern, 0)); + } + return; + } case MATCH_DUP: case MATCH_OP_DUP: case MATCH_PAR_DUP: @@ -467,18 +504,14 @@ { const char constraints0 = XSTR (pattern, 2)[0]; - /* In DEFINE_EXPAND, DEFINE_SPLIT, and DEFINE_PEEPHOLE2, we - don't use the MATCH_OPERAND constraint, only the predicate. - This is confusing to folks doing new ports, so help them - not make the mistake. */ - if (GET_CODE (insn) == DEFINE_EXPAND - || GET_CODE (insn) == DEFINE_SPLIT - || GET_CODE (insn) == DEFINE_PEEPHOLE2) + if (!constraints_supported_in_insn_p (insn)) { if (constraints0) - error_with_line (pattern_lineno, - "constraints not supported in %s", - rtx_name[GET_CODE (insn)]); + { + error_with_line (pattern_lineno, + "constraints not supported in %s", + rtx_name[GET_CODE (insn)]); + } } /* A MATCH_OPERAND that is a SET should have an output reload. */ Index: gcc/doc/md.texi =================================================================== --- gcc/doc/md.texi (revision 215387) +++ gcc/doc/md.texi (working copy) @@ -1546,18 +1546,18 @@ @table @samp @cindex @samp{=} in constraint @item = -Means that this operand is write-only for this instruction: the previous -value is discarded and replaced by output data. +Means that this operand is written to by this instruction: +the previous value is discarded and replaced by new data. @cindex @samp{+} in constraint @item + Means that this operand is both read and written by the instruction. When the compiler fixes up the operands to satisfy the constraints, -it needs to know which operands are inputs to the instruction and -which are outputs from it. @samp{=} identifies an output; @samp{+} -identifies an operand that is both input and output; all other operands -are assumed to be input only. +it needs to know which operands are read by the instruction and +which are written by it. @samp{=} identifies an operand which is only +written; @samp{+} identifies an operand that is both read and written; all +other operands are assumed to only be read. If you specify @samp{=} or @samp{+} in a constraint, you put it in the first character of the constraint string. @@ -1566,9 +1566,9 @@ @cindex earlyclobber operand @item & Means (in a particular alternative) that this operand is an -@dfn{earlyclobber} operand, which is modified before the instruction is +@dfn{earlyclobber} operand, which is written before the instruction is finished using the input operands. Therefore, this operand may not lie -in a register that is used as an input operand or as part of any memory +in a register that is read by the instruction or as part of any memory address. @samp{&} applies only to the alternative in which it is written. In @@ -1576,16 +1576,19 @@ requires @samp{&} while others do not. See, for example, the @samp{movdf} insn of the 68000. -An input operand can be tied to an earlyclobber operand if its only -use as an input occurs before the early result is written. Adding -alternatives of this form often allows GCC to produce better code -when only some of the inputs can be affected by the earlyclobber. -See, for example, the @samp{mulsi3} insn of the ARM@. +A operand which is read by the instruction can be tied to an earlyclobber +operand if its only use as an input occurs before the early result is +written. Adding alternatives of this form often allows GCC to produce +better code when only some of the read operands can be affected by the +earlyclobber. See, for example, the @samp{mulsi3} insn of the ARM@. -Furthermore, if the @dfn{earlyclobber} operand is also read/write operand, then -that operand is modified only after it's used. +Furthermore, if the @dfn{earlyclobber} operand is also a read/write +operand, then that operand is written only after it's used. -@samp{&} does not obviate the need to write @samp{=} or @samp{+}. +@samp{&} does not obviate the need to write @samp{=} or @samp{+}. As +@dfn{earlyclobber} operands are always written, a read-only +@dfn{earlyclobber} operand is ill-formed and will be rejected by the +compiler. @cindex @samp{%} in constraint @item % @@ -1593,7 +1596,7 @@ following operand. This means that the compiler may interchange the two operands if that is the cheapest way to make all operands fit the constraints. @samp{%} applies to all alternatives and must appear as -the first character in the constraint. Only input operands can use +the first character in the constraint. Only read-only operands can use @samp{%}. @ifset INTERNALS --T4sUOijqQbZv57TR--