Re: [Patch] Teach genrecog/genoutput that scratch registers require write constraint modifiers

[James Greenhalgh <james dot greenhalgh at arm dot com>]

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

Thanks Jeff,

In the end I committed the attached as revision 215388.

Cheers,
James

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 215387)
+++ gcc/ChangeLog	(working copy)
@@ -1,5 +1,15 @@
 2014-09-19  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.
+
+2014-09-19  James Greenhalgh  <james.greenhalgh@arm.com>
+
 	* config/aarch64/aarch64.md (stack_protect_test_<mode>): 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