diff mbox

Teach genrecog/genoutput that scratch registers require write constraint modifiers

Message ID 1411035561-17445-1-git-send-email-james.greenhalgh@arm.com
State New
Headers show

Commit Message

James Greenhalgh Sept. 18, 2014, 10:19 a.m. UTC
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  <james.greenhalgh@arm.com>

	* 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.

Comments

Jeff Law Sept. 18, 2014, 8:45 p.m. UTC | #1
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  <james.greenhalgh@arm.com>
>
> 	* 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.

Jeff
Jeff Law Sept. 18, 2014, 9:36 p.m. UTC | #2
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  <james.greenhalgh@arm.com>
>
> 	* 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.
OK.
Jeff
Jan-Benedict Glaw Sept. 20, 2014, 7:40 p.m. UTC | #3
Hi!

On Thu, 2014-09-18 11:19:21 +0100, James Greenhalgh <james.greenhalgh@arm.com> wrote:
> 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.

My build robot shows a new build error, which I guess is
caused/uncovered by your genrecog change on bfin-elf (see eg. build
http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=355667):

build/genrecog /home/jbglaw/repos/gcc/gcc/common.md /home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md \
  insn-conditions.md > tmp-recog.c
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:745: warning: operand 0 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:1953: warning: source missing a mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:1994: warning: source missing a mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:1994: warning: source missing a mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2005: warning: source missing a mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2005: warning: source missing a mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2128: warning: operand 1 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2139: warning: operand 1 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2150: warning: operand 2 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2162: warning: operand 2 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2228: warning: operand 1 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2238: warning: operand 1 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2248: warning: operand 2 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2259: warning: operand 2 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2720: warning: operand 0 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2742: warning: operand 0 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:2742: warning: operand 1 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:3231: warning: operand 4 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:3247: warning: operand 3 missing mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:1971: warning: source missing a mode?
/home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:1971: constraints not supported in define_split
make[1]: *** [s-recog] Error 1
make[1]: Leaving directory `/home/jbglaw/build/bfin-elf/build-gcc/gcc'
make: *** [all-gcc] Error 2


Would be nice if the bfin maintainer or you would come up with a fix.

Thanks, JBG
James Greenhalgh Sept. 22, 2014, 7:58 a.m. UTC | #4
On Sat, Sep 20, 2014 at 08:40:01PM +0100, Jan-Benedict Glaw wrote:
> Hi!
> 
> On Thu, 2014-09-18 11:19:21 +0100, James Greenhalgh <james.greenhalgh@arm.com> wrote:
> > 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.
> 
> My build robot shows a new build error, which I guess is
> caused/uncovered by your genrecog change on bfin-elf (see eg. build
> http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=355667):
> 
> build/genrecog /home/jbglaw/repos/gcc/gcc/common.md /home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md \
>   insn-conditions.md > tmp-recog.c
> /home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:1971: constraints not supported in define_split
> make[1]: *** [s-recog] Error 1
> make[1]: Leaving directory `/home/jbglaw/build/bfin-elf/build-gcc/gcc'
> make: *** [all-gcc] Error 2
> 
> 
> Would be nice if the bfin maintainer or you would come up with a fix.

Hi Jan,

I posted a fix for this on Friday evening at:

  https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01682.html

I'm waiting for a bfin maintainer to say OK, as it isn't a port I know
well.

Thanks,
James
Jan-Benedict Glaw Sept. 22, 2014, 9:08 a.m. UTC | #5
On Mon, 2014-09-22 08:58:34 +0100, James Greenhalgh <james.greenhalgh@arm.com> wrote:
> On Sat, Sep 20, 2014 at 08:40:01PM +0100, Jan-Benedict Glaw wrote:
> > My build robot shows a new build error, which I guess is
> > caused/uncovered by your genrecog change on bfin-elf (see eg. build
> > http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=355667):
[...]
> > build/genrecog /home/jbglaw/repos/gcc/gcc/common.md /home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md \
> >   insn-conditions.md > tmp-recog.c
> > /home/jbglaw/repos/gcc/gcc/config/bfin/bfin.md:1971: constraints not supported in define_split
> > make[1]: *** [s-recog] Error 1
> > make[1]: Leaving directory `/home/jbglaw/build/bfin-elf/build-gcc/gcc'
> > make: *** [all-gcc] Error 2
> > 
> > Would be nice if the bfin maintainer or you would come up with a fix.
> 
> I posted a fix for this on Friday evening at:
> 
>   https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01682.html
> 
> I'm waiting for a bfin maintainer to say OK, as it isn't a port I know
> well.

Ah great!  I somehow missed to recognize your email, sorry for the
noise.

MfG, JBG
diff mbox

Patch

diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
index 80e8bd6..435d850 100644
--- a/gcc/doc/md.texi
+++ b/gcc/doc/md.texi
@@ -1546,18 +1546,18 @@  Here are constraint modifier characters.
 @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 @@  first character of the constraint string.
 @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 @@  constraints with multiple alternatives, sometimes one alternative
 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 @@  Declares the instruction to be commutative for this operand and the
 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
diff --git a/gcc/genoutput.c b/gcc/genoutput.c
index 69d5ab0..8094288 100644
--- a/gcc/genoutput.c
+++ b/gcc/genoutput.c
@@ -769,6 +769,7 @@  validate_insn_alternatives (struct data *d)
 	char c;
 	int which_alternative = 0;
 	int alternative_count_unsure = 0;
+	bool seen_inout = false;
 
 	for (p = d->operand[start].constraint; (c = *p); p += len)
 	  {
@@ -777,6 +778,18 @@  validate_insn_alternatives (struct data *d)
 	      error_with_line (d->lineno,
 			       "character '%c' can only be used at the"
 			       " beginning of a constraint string", c);
+
+	    if (c == '=' || c == '+')
+	      seen_inout = true;
+
+	    /* Earlyclobber operands must always be marked write-only
+	       or read-write.  */
+	    if (!seen_inout && 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))
diff --git a/gcc/genrecog.c b/gcc/genrecog.c
index dbdefb0..4de4718 100644
--- a/gcc/genrecog.c
+++ b/gcc/genrecog.c
@@ -415,6 +415,18 @@  find_matching_operand (rtx pattern, int n)
   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
@@ -432,7 +444,33 @@  validate_pattern (rtx pattern, rtx insn, rtx set, int set_code)
   switch (code)
     {
     case MATCH_SCRATCH:
-      return;
+      {
+	/* If a MATCH_SCRATCH is used in a context requiring an output
+	   register, validate that.  If it is used in a context requiring
+	   an in/out register, that is an error.  */
+	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 (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 +505,14 @@  validate_pattern (rtx pattern, rtx insn, rtx set, int set_code)
 	  {
 	    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.  */