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

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

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]


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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-Teach-genrecog-genoutput-that-scratch-register.patch --]
[-- Type: text/x-patch;  name=0001-Patch-Teach-genrecog-genoutput-that-scratch-register.patch, Size: 7640 bytes --]

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

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

[Jeff Law]

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

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

[Jeff Law]

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

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

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

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


[-- Attachment #2: 0001-Patch-Teach-genrecog-genoutput-that-scratch-register.patch --]
[-- Type: text/x-diff, Size: 7842 bytes --]

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

[Patch sh] Fixup use of constraints in define_split

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]


Hi,

After https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01615.html we error
on the use of constraints in define_splits, define_expands and
define_peephole2s. These are never looked at by the compiler, and so
have no reason to be set.

I expect there will be more fallout as Jan's auto-builder makes its way
through ports I haven't tested, I'll fix those up as they come up.

These are build failures, and the fixes are "obvious", but I don't know
my way around these ports, so I'd like an explicit maintainer ack.

For testing, I've just checked that the build error is resolved. In
principal, these are not functional changes as the constraints are
not looked at.

OK?

Thanks,
James

---
2014-09-19  James Greenhalgh  <james.greenhalgh@arm.com>

	* config/sh/sh.md: Fix use of constraints in define_split.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-sh-Fixup-use-of-constraints-in-define_split.patch --]
[-- Type: text/x-patch;  name=0001-Patch-sh-Fixup-use-of-constraints-in-define_split.patch, Size: 894 bytes --]

diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
index 56dee824f1c0674d51224a3e3b9be20bec7920cc..4bee5cac2ea35b12a7de4cbc3e11f0c71becf996 100644
--- a/gcc/config/sh/sh.md
+++ b/gcc/config/sh/sh.md
@@ -7709,10 +7709,10 @@ (define_insn "movdf_i4"
 ;; use that will prevent scheduling of other stack accesses beyond this
 ;; instruction.
 (define_split
-  [(set (match_operand:DF 0 "register_operand" "")
-	(match_operand:DF 1 "register_operand" ""))
-   (use (match_operand:PSI 2 "fpscr_operand" ""))
-   (clobber (match_scratch:SI 3 "=X"))]
+  [(set (match_operand:DF 0 "register_operand")
+	(match_operand:DF 1 "register_operand"))
+   (use (match_operand:PSI 2 "fpscr_operand"))
+   (clobber (match_scratch:SI 3))]
   "(TARGET_SH4 || TARGET_SH2A_DOUBLE) && reload_completed
    && (true_regnum (operands[0]) < 16) != (true_regnum (operands[1]) < 16)"
   [(const_int 0)]

Re: [Patch sh] Fixup use of constraints in define_split

[Jeff Law]

On 09/19/14 06:59, James Greenhalgh wrote:
>
> Hi,
>
> After https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01615.html we error
> on the use of constraints in define_splits, define_expands and
> define_peephole2s. These are never looked at by the compiler, and so
> have no reason to be set.
Right.

>
> I expect there will be more fallout as Jan's auto-builder makes its way
> through ports I haven't tested, I'll fix those up as they come up.
Yes.

>
> These are build failures, and the fixes are "obvious", but I don't know
> my way around these ports, so I'd like an explicit maintainer ack.
No problem, someone should be able to ack these very quickly since they 
don't affect correctness/functionality at all.

>
> For testing, I've just checked that the build error is resolved. In
> principal, these are not functional changes as the constraints are
> not looked at.
>
> OK?
>
> Thanks,
> James
>
> ---
> 2014-09-19  James Greenhalgh  <james.greenhalgh@arm.com>
>
> 	* config/sh/sh.md: Fix use of constraints in define_split.
Ok.

jeff

Re: [Patch sh] Fixup use of constraints in define_split

[Andreas Krebbel]

On 09/19/2014 02:59 PM, James Greenhalgh wrote:
> 
> Hi,
> 
> After https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01615.html we error
> on the use of constraints in define_splits, define_expands and
> define_peephole2s. These are never looked at by the compiler, and so
> have no reason to be set.
> 
> I expect there will be more fallout as Jan's auto-builder makes its way
> through ports I haven't tested, I'll fix those up as they come up.
> 
> These are build failures, and the fixes are "obvious", but I don't know
> my way around these ports, so I'd like an explicit maintainer ack.
> 
> For testing, I've just checked that the build error is resolved. In
> principal, these are not functional changes as the constraints are
> not looked at.

S/390 bootstrap fails with:
gcc/gcc/config/s390/s390.md:8397: operand 5 missing output reload

For the branch on index instruction we have a define_insn_and_split with a single alternative
forcing a split before reload. In this pattern to my understanding the constraints are not really
required. However, for the new checker enhancements I would apply the following to avoid the error:

diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
index 73ac0dc..06aaced 100644
--- a/gcc/config/s390/s390.md
+++ b/gcc/config/s390/s390.md
@@ -8405,7 +8405,7 @@
         (pc)))
    (set (match_operand:GPR 4 "nonimmediate_operand" "")
         (plus:GPR (match_dup 1) (match_dup 2)))
-   (clobber (match_scratch:GPR 5 ""))]
+   (clobber (match_scratch:GPR 5 "=d"))]
   "TARGET_CPU_ZARCH"
   "#"
   "!reload_completed && !reload_in_progress"

-Andreas-

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

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]


On Fri, Sep 19, 2014 at 04:10:34PM +0100, Andreas Krebbel wrote:
> On 09/19/2014 02:59 PM, James Greenhalgh wrote:
> >
> > Hi,
> >
> > After https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01615.html we error
> > on the use of constraints in define_splits, define_expands and
> > define_peephole2s. These are never looked at by the compiler, and so
> > have no reason to be set.
> >
> > I expect there will be more fallout as Jan's auto-builder makes its way
> > through ports I haven't tested, I'll fix those up as they come up.
> >
> > These are build failures, and the fixes are "obvious", but I don't know
> > my way around these ports, so I'd like an explicit maintainer ack.
> >
> > For testing, I've just checked that the build error is resolved. In
> > principal, these are not functional changes as the constraints are
> > not looked at.
>
> S/390 bootstrap fails with:
> gcc/gcc/config/s390/s390.md:8397: operand 5 missing output reload
>
> For the branch on index instruction we have a define_insn_and_split with a
> single alternative forcing a split before reload. In this pattern to my
> understanding the constraints are not really required.

Yes, if that is the existing supported behaviour for MATCH_OPERANDs (it
must be or your pattern would have been failing before), I don't want to
make MATCH_SCRATCH more restrictive.

The fix is a one-liner. Jeff, I presume we still want to permit this?
The alternate fix would be to make this an error in both cases.

Ok?

Thanks,
James

---
2014-09-14  James Greenhalgh  <james.greenhalgh@arm.com>

	* genrecog.c (validate_pattern): Allow empty constraints in
	a match_scratch.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Re-Patch-Teach-genrecog-genoutput-that-scratch-regis.patch --]
[-- Type: text/x-patch;  name=0001-Re-Patch-Teach-genrecog-genoutput-that-scratch-regis.patch, Size: 437 bytes --]

diff --git a/gcc/genrecog.c b/gcc/genrecog.c
index 0134585..c1dc8fa 100644
--- a/gcc/genrecog.c
+++ b/gcc/genrecog.c
@@ -461,6 +461,7 @@ validate_pattern (rtx pattern, rtx insn, rtx set, int set_code)
 	/* If a MATCH_SCRATCH is used in a context requiring an write-only
 	   or read/write register, validate that.  */
 	if (set_code == '='
+	    && constraints0
 	    && constraints0 != '='
 	    && constraints0 != '+')
 	  {

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

[Jeff Law]

On 09/19/14 10:16, James Greenhalgh wrote:
>
> On Fri, Sep 19, 2014 at 04:10:34PM +0100, Andreas Krebbel wrote:
>> On 09/19/2014 02:59 PM, James Greenhalgh wrote:
>>>
>>> Hi,
>>>
>>> After https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01615.html we error
>>> on the use of constraints in define_splits, define_expands and
>>> define_peephole2s. These are never looked at by the compiler, and so
>>> have no reason to be set.
>>>
>>> I expect there will be more fallout as Jan's auto-builder makes its way
>>> through ports I haven't tested, I'll fix those up as they come up.
>>>
>>> These are build failures, and the fixes are "obvious", but I don't know
>>> my way around these ports, so I'd like an explicit maintainer ack.
>>>
>>> For testing, I've just checked that the build error is resolved. In
>>> principal, these are not functional changes as the constraints are
>>> not looked at.
>>
>> S/390 bootstrap fails with:
>> gcc/gcc/config/s390/s390.md:8397: operand 5 missing output reload
>>
>> For the branch on index instruction we have a define_insn_and_split with a
>> single alternative forcing a split before reload. In this pattern to my
>> understanding the constraints are not really required.
>
> Yes, if that is the existing supported behaviour for MATCH_OPERANDs (it
> must be or your pattern would have been failing before), I don't want to
> make MATCH_SCRATCH more restrictive.
>
> The fix is a one-liner. Jeff, I presume we still want to permit this?
> The alternate fix would be to make this an error in both cases.
>
> Ok?
>
> Thanks,
> James
>
> ---
> 2014-09-14  James Greenhalgh  <james.greenhalgh@arm.com>
>
> 	* genrecog.c (validate_pattern): Allow empty constraints in
> 	a match_scratch.
OK.  Yea, I think we want to permit -- I have vague recollections this 
is documented somewhere as "accept anything".

jeff

[Patch bfin] Fixup use of constraints in define_split

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

Hi,

As with the earlier patch for sh
( https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01627.html ), this fixes the
fallout caused by https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01615.html.

These are build failures, and the fixes are "obvious", but I don't know
my way around the failing ports, so I'd like an explicit maintainer ack.

For testing, I've just checked that the build error is resolved.

Ok?

Thanks,
James

---
2014-09-19  James Greenhalgh  <james.greenhalgh@arm.com>

	* config/bfin/bfin.md: Fix use of constraints in define_split.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-bfin-Fixup-use-of-constraints-in-define_split.patch --]
[-- Type: text/x-patch;  name=0001-Patch-bfin-Fixup-use-of-constraints-in-define_split.patch, Size: 945 bytes --]

diff --git a/gcc/config/bfin/bfin.md b/gcc/config/bfin/bfin.md
index f5e64d3ef6914b408fa68b044ad122e676e2d7ff..9d57b9d3d392179effb68c1a511afaf8e0b43462 100644
--- a/gcc/config/bfin/bfin.md
+++ b/gcc/config/bfin/bfin.md
@@ -1970,15 +1970,15 @@ (define_insn "loop_end"
 
 (define_split
   [(set (pc)
-	(if_then_else (ne (match_operand:SI 0 "nondp_reg_or_memory_operand" "")
+	(if_then_else (ne (match_operand:SI 0 "nondp_reg_or_memory_operand")
 			  (const_int 1))
-		      (label_ref (match_operand 1 "" ""))
+		      (label_ref (match_operand 1 ""))
 		      (pc)))
    (set (match_dup 0)
 	(plus (match_dup 0)
 	      (const_int -1)))
    (unspec [(const_int 0)] UNSPEC_LSETUP_END)
-   (clobber (match_scratch:SI 2 "=&r"))]
+   (clobber (match_scratch:SI 2))]
   "memory_operand (operands[0], SImode) || splitting_loops"
   [(set (match_dup 2) (match_dup 0))
    (set (match_dup 2) (plus:SI (match_dup 2) (const_int -1)))

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

[Andreas Schwab]

Tested on ia64-suse-linux and checked in as obvious.

Andreas.

	* config/ia64/ia64.md: Remove constraints from define_split
	patterns.

diff --git a/gcc/config/ia64/ia64.md b/gcc/config/ia64/ia64.md
index 986ef02..572870f 100644
--- a/gcc/config/ia64/ia64.md
+++ b/gcc/config/ia64/ia64.md
@@ -2565,7 +2565,7 @@
   [(set (match_operand:TI 0 "register_operand" "")
 	(minus:TI (match_operand:TI 1 "register_operand" "")
 		  (match_operand:TI 2 "register_operand" "")))
-   (clobber (match_scratch:BI 3 "=&c"))]
+   (clobber (match_scratch:BI 3 ""))]
   "reload_completed"
   [(set (match_dup 0) (minus:DI (match_dup 1) (match_dup 2)))
    (set (match_dup 3) (ltu:BI (match_dup 1) (match_dup 0)))
@@ -2587,7 +2587,7 @@
   [(set (match_operand:TI 0 "register_operand" "")
 	(minus:TI (match_operand:TI 1 "immediate_operand" "")
 		  (match_operand:TI 2 "register_operand" "")))
-   (clobber (match_scratch:BI 3 "=&c"))]
+   (clobber (match_scratch:BI 3 ""))]
   "reload_completed && satisfies_constraint_K (operands[1])"
   [(set (match_dup 0) (minus:DI (match_dup 1) (match_dup 2)))
    (set (match_dup 3) (gtu:BI (match_dup 0) (match_dup 1)))
@@ -4152,7 +4152,7 @@
   [(set (match_operand:DI 0 "register_operand" "")
 	(if_then_else:DI
 	  (match_operator 4 "predicate_operator"
-	    [(match_operand:BI 1 "register_operand" "c,c")
+	    [(match_operand:BI 1 "register_operand" "")
 	     (const_int 0)])
 	  (neg:DI (match_operand:DI 2 "gr_reg_or_22bit_operand" ""))
 	  (match_operand:DI 3 "gr_reg_or_22bit_operand" "")))]
@@ -4167,7 +4167,7 @@
   [(set (match_operand:DI 0 "register_operand" "")
 	(if_then_else:DI
 	  (match_operator 4 "predicate_operator"
-	    [(match_operand:BI 1 "register_operand" "c,c")
+	    [(match_operand:BI 1 "register_operand" "")
 	     (const_int 0)])
 	  (neg:DI (match_operand:DI 2 "gr_reg_or_22bit_operand" ""))
 	  (match_operand:DI 3 "gr_reg_or_22bit_operand" "")))]
@@ -4220,7 +4220,7 @@
   [(set (match_operand:SI 0 "register_operand" "")
 	(if_then_else:SI
 	  (match_operator 4 "predicate_operator"
-	    [(match_operand:BI 1 "register_operand" "c,c")
+	    [(match_operand:BI 1 "register_operand" "")
 	     (const_int 0)])
 	  (neg:SI (match_operand:SI 2 "gr_reg_or_22bit_operand" ""))
 	  (match_operand:SI 3 "gr_reg_or_22bit_operand" "")))]
@@ -4235,7 +4235,7 @@
   [(set (match_operand:SI 0 "register_operand" "")
 	(if_then_else:SI
 	  (match_operator 4 "predicate_operator"
-	    [(match_operand:BI 1 "register_operand" "c,c")
+	    [(match_operand:BI 1 "register_operand" "")
 	     (const_int 0)])
 	  (neg:SI (match_operand:SI 2 "gr_reg_or_22bit_operand" ""))
 	  (match_operand:SI 3 "gr_reg_or_22bit_operand" "")))]
-- 
2.1.0

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[BUILDROBOT] genrecog fix uncovers problem in bfin.md (was: [Patch] Teach genrecog/genoutput that scratch registers require write constraint modifiers)

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 3032 bytes --]

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

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
Signature of:            http://www.chiark.greenend.org.uk/~sgtatham/bugs.html
the second  :

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Patch sh] Fixup use of constraints in define_split

[Oleg Endo]

On Fri, 2014-09-19 at 13:59 +0100, James Greenhalgh wrote:
> Hi,
> 
> After https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01615.html we error
> on the use of constraints in define_splits, define_expands and
> define_peephole2s. These are never looked at by the compiler, and so
> have no reason to be set.
> 
> I expect there will be more fallout as Jan's auto-builder makes its way
> through ports I haven't tested, I'll fix those up as they come up.
> 
> These are build failures, and the fixes are "obvious", but I don't know
> my way around these ports, so I'd like an explicit maintainer ack.
> 
> For testing, I've just checked that the build error is resolved. In
> principal, these are not functional changes as the constraints are
> not looked at.
> 
> OK?

OK for trunk.

Cheers,
Oleg

Re: [BUILDROBOT] genrecog fix uncovers problem in bfin.md (was: [Patch] Teach genrecog/genoutput that scratch registers require write constraint modifiers)

[James Greenhalgh]

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

Re: [BUILDROBOT] genrecog fix uncovers problem in bfin.md (was: [Patch] Teach genrecog/genoutput that scratch registers require write constraint modifiers)

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

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

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
Signature of:            http://www.chiark.greenend.org.uk/~sgtatham/bugs.html
the second  :

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Patch bfin] Fixup use of constraints in define_split

[Bernd Schmidt]

On 09/19/2014 11:32 PM, James Greenhalgh wrote:
> As with the earlier patch for sh
> ( https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01627.html ), this fixes the
> fallout caused by https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01615.html.
>
> These are build failures, and the fixes are "obvious", but I don't know
> my way around the failing ports, so I'd like an explicit maintainer ack.
>
> For testing, I've just checked that the build error is resolved.

Looks obvious to me too. Thanks!


Bernd