[PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

[PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

[Jakub Jelinek]

Hi!

Right now, stmt.c on constraints not hardcoded in it, and not
define_{register,address,memory}_constraint just assumes the
constraint might allow both reg and mem.  Unfortunately, on some
constraints which clearly can't allow either of those leads to errors
at -O0, because the expander doesn't try so hard to expand it as
EXPAND_INITIALIZER.

The following patch is an attempt to handle at least the easy cases
- define_constraint like:
(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)")))
where the match_code clearly proves that it never can match any REG/SUBREG,
nor MEM, by teaching genpreds.c to emit an extra inline function that
stmt.c can in process_{output,input}_constraint use for the unknown
constraints.

On x86_64/i686 this only detects constraint G as not allowing reg nor mem
(it is match_code const_double), and V (plus < and >, but those are
hardcoded in stmt.c already) that it allows mem but not reg.
On aarch64, in the first category it detects several constraints.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2015-04-08  Jakub Jelinek  <jakub@redhat.com>

	PR target/65689
	* genpreds.c (struct constraint_data): Add maybe_allows_reg and
	maybe_allows_mem bitfields.
	(maybe_allows_none_start, maybe_allows_none_end,
	maybe_allows_reg_start, maybe_allows_reg_end, maybe_allows_mem_start,
	maybe_allows_mem_end): New variables.
	(compute_maybe_allows): New function.
	(add_constraint): Use it to initialize maybe_allows_reg and
	maybe_allows_mem fields.
	(choose_enum_order): Sort the non-is_register/is_const_int/is_memory/
	is_address constraints such that those that allow neither mem nor
	reg come first, then those that only allow reg but not mem, then
	those that only allow mem but not reg, then the rest.
	(write_allows_reg_mem_function): New function.
	(write_tm_preds_h): Call it.
	* stmt.c (parse_output_constraint, parse_input_constraint): Use
	the generated insn_extra_constraint_allows_reg_mem function
	instead of always setting *allows_reg = true; *allows_mem = true;
	for unknown extra constraints.

--- gcc/genpreds.c.jj	2015-01-09 21:59:54.000000000 +0100
+++ gcc/genpreds.c	2015-04-08 14:09:51.713934240 +0200
@@ -640,12 +640,14 @@ struct constraint_data
   const char *regclass;  /* for register constraints */
   rtx exp;               /* for other constraints */
   unsigned int lineno;   /* line of definition */
-  unsigned int is_register  : 1;
-  unsigned int is_const_int : 1;
-  unsigned int is_const_dbl : 1;
-  unsigned int is_extra     : 1;
-  unsigned int is_memory    : 1;
-  unsigned int is_address   : 1;
+  unsigned int is_register	: 1;
+  unsigned int is_const_int	: 1;
+  unsigned int is_const_dbl	: 1;
+  unsigned int is_extra		: 1;
+  unsigned int is_memory	: 1;
+  unsigned int is_address	: 1;
+  unsigned int maybe_allows_reg : 1;
+  unsigned int maybe_allows_mem : 1;
 };
 
 /* Overview of all constraints beginning with a given letter.  */
@@ -691,6 +693,9 @@ static unsigned int satisfied_start;
 static unsigned int const_int_start, const_int_end;
 static unsigned int memory_start, memory_end;
 static unsigned int address_start, address_end;
+static unsigned int maybe_allows_none_start, maybe_allows_none_end;
+static unsigned int maybe_allows_reg_start, maybe_allows_reg_end;
+static unsigned int maybe_allows_mem_start, maybe_allows_mem_end;
 
 /* Convert NAME, which contains angle brackets and/or underscores, to
    a string that can be used as part of a C identifier.  The string
@@ -711,6 +716,46 @@ mangle (const char *name)
   return XOBFINISH (rtl_obstack, const char *);
 }
 
+/* Return a bitmask, bit 1 if EXP maybe allows a REG/SUBREG, 2 if EXP
+   maybe allows a MEM.  This should be conservative.  */
+static int
+compute_maybe_allows (rtx exp)
+{
+  switch (GET_CODE (exp))
+    {
+    case IF_THEN_ELSE:
+      /* Conservative answer is like IOR, of the THEN and ELSE branches.  */
+      return compute_maybe_allows (XEXP (exp, 1))
+	     | compute_maybe_allows (XEXP (exp, 2));
+    case AND:
+      return compute_maybe_allows (XEXP (exp, 0))
+	     & compute_maybe_allows (XEXP (exp, 1));
+    case IOR:
+      return compute_maybe_allows (XEXP (exp, 0))
+	     | compute_maybe_allows (XEXP (exp, 1));
+    case MATCH_CODE:
+      if (*XSTR (exp, 1) == '\0')
+	{
+	  const char *code, *codes = XSTR (exp, 0);
+	  int ret = 0;
+	  while ((code = scan_comma_elt (&codes)) != 0)
+	    if (strncmp (code, "reg", 3) == 0
+		&& (code[3] == ',' || code[3] == '\0' || code[3] == ' '))
+	      ret |= 1;
+	    else if (strncmp (code, "subreg", 6) == 0
+		     && (code[6] == ',' || code[6] == '\0' || code[6] == ' '))
+	      ret |= 1;
+	    else if (strncmp (code, "mem", 3) == 0
+		     && (code[3] == ',' || code[3] == '\0' || code[3] == ' '))
+	      ret |= 2;
+	  return ret;
+	}
+      /* FALLTHRU */
+    default:
+      return 3;
+    }
+}
+
 /* Add one constraint, of any sort, to the tables.  NAME is its name;
    REGCLASS is the register class, if any; EXP is the expression to
    test, if any;  IS_MEMORY and IS_ADDRESS indicate memory and address
@@ -866,6 +911,11 @@ add_constraint (const char *name, const
   c->is_extra = !(regclass || is_const_int || is_const_dbl);
   c->is_memory = is_memory;
   c->is_address = is_address;
+  int maybe_allows = 3;
+  if (exp)
+    maybe_allows = compute_maybe_allows (exp);
+  c->maybe_allows_reg = (maybe_allows & 1) != 0;
+  c->maybe_allows_mem = (maybe_allows & 2) != 0;
 
   c->next_this_letter = *slot;
   *slot = c;
@@ -940,8 +990,30 @@ choose_enum_order (void)
       enum_order[next++] = c;
   address_end = next;
 
+  maybe_allows_none_start = next;
+  FOR_ALL_CONSTRAINTS (c)
+    if (!c->is_register && !c->is_const_int && !c->is_memory && !c->is_address
+	&& !c->maybe_allows_reg && !c->maybe_allows_mem)
+      enum_order[next++] = c;
+  maybe_allows_none_end = next;
+
+  maybe_allows_reg_start = next;
   FOR_ALL_CONSTRAINTS (c)
-    if (!c->is_register && !c->is_const_int && !c->is_memory && !c->is_address)
+    if (!c->is_register && !c->is_const_int && !c->is_memory && !c->is_address
+	&& c->maybe_allows_reg && !c->maybe_allows_mem)
+      enum_order[next++] = c;
+  maybe_allows_reg_end = next;
+
+  maybe_allows_mem_start = next;
+  FOR_ALL_CONSTRAINTS (c)
+    if (!c->is_register && !c->is_const_int && !c->is_memory && !c->is_address
+	&& !c->maybe_allows_reg && c->maybe_allows_mem)
+      enum_order[next++] = c;
+  maybe_allows_mem_end = next;
+
+  FOR_ALL_CONSTRAINTS (c)
+    if (!c->is_register && !c->is_const_int && !c->is_memory && !c->is_address
+	&& c->maybe_allows_reg && c->maybe_allows_mem)
       enum_order[next++] = c;
   gcc_assert (next == num_constraints);
 }
@@ -1229,6 +1301,41 @@ write_range_function (const char *name,
 	    "}\n\n", name);
 }
 
+/* Write a definition for insn_extra_constraint_allows_reg_mem function.  */
+static void
+write_allows_reg_mem_function (void)
+{
+  printf ("static inline void\n"
+	  "insn_extra_constraint_allows_reg_mem (enum constraint_num c,\n"
+	  "\t\t\t\t      bool *allows_reg, bool *allows_mem)\n"
+	  "{\n");
+  if (maybe_allows_none_start != maybe_allows_none_end)
+    printf ("  if (c >= CONSTRAINT_%s && c <= CONSTRAINT_%s)\n"
+	    "    return;\n",
+	    enum_order[maybe_allows_none_start]->c_name,
+	    enum_order[maybe_allows_none_end - 1]->c_name);
+  if (maybe_allows_reg_start != maybe_allows_reg_end)
+    printf ("  if (c >= CONSTRAINT_%s && c <= CONSTRAINT_%s)\n"
+	    "    {\n"
+	    "      *allows_reg = true;\n"
+	    "      return;\n"
+	    "    }\n",
+	    enum_order[maybe_allows_reg_start]->c_name,
+	    enum_order[maybe_allows_reg_end - 1]->c_name);
+  if (maybe_allows_mem_start != maybe_allows_mem_end)
+    printf ("  if (c >= CONSTRAINT_%s && c <= CONSTRAINT_%s)\n"
+	    "    {\n"
+	    "      *allows_mem = true;\n"
+	    "      return;\n"
+	    "    }\n",
+	    enum_order[maybe_allows_mem_start]->c_name,
+	    enum_order[maybe_allows_mem_end - 1]->c_name);
+  printf ("  (void) c;\n"
+	  "  *allows_reg = true;\n"
+	  "  *allows_mem = true;\n"
+	  "}\n\n");
+}
+
 /* VEC is a list of key/value pairs, with the keys being lower bounds
    of a range.  Output a decision tree that handles the keys covered by
    [VEC[START], VEC[END]), returning FALLBACK for keys lower then VEC[START]'s.
@@ -1326,6 +1433,7 @@ write_tm_preds_h (void)
 			    memory_start, memory_end);
       write_range_function ("insn_extra_address_constraint",
 			    address_start, address_end);
+      write_allows_reg_mem_function ();
 
       if (constraint_max_namelen > 1)
         {
--- gcc/stmt.c.jj	2015-02-13 15:41:11.000000000 +0100
+++ gcc/stmt.c	2015-04-08 14:06:37.279155475 +0200
@@ -342,13 +342,7 @@ parse_output_constraint (const char **co
 	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;
-	  }
+	  insn_extra_constraint_allows_reg_mem (cn, allows_reg, allows_mem);
 	break;
       }
 
@@ -465,13 +459,7 @@ parse_input_constraint (const char **con
 	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;
-	  }
+	  insn_extra_constraint_allows_reg_mem (cn, allows_reg, allows_mem);
 	break;
       }
 

	Jakub

Re: [PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

[Segher Boessenkool]

On Wed, Apr 08, 2015 at 11:00:59PM +0200, Jakub Jelinek wrote:
> +    case MATCH_CODE:
> +      if (*XSTR (exp, 1) == '\0')
> +	{
> +	  const char *code, *codes = XSTR (exp, 0);
> +	  int ret = 0;
> +	  while ((code = scan_comma_elt (&codes)) != 0)
> +	    if (strncmp (code, "reg", 3) == 0
> +		&& (code[3] == ',' || code[3] == '\0' || code[3] == ' '))

This doesn't allow other whitespace.  Maybe it's cleaner written as e.g.

	&& codes - code == 3

?


Segher

Re: [PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

[Segher Boessenkool]

On Wed, Apr 08, 2015 at 05:12:07PM -0500, Segher Boessenkool wrote:
> On Wed, Apr 08, 2015 at 11:00:59PM +0200, Jakub Jelinek wrote:
> > +    case MATCH_CODE:
> > +      if (*XSTR (exp, 1) == '\0')
> > +	{
> > +	  const char *code, *codes = XSTR (exp, 0);
> > +	  int ret = 0;
> > +	  while ((code = scan_comma_elt (&codes)) != 0)
> > +	    if (strncmp (code, "reg", 3) == 0
> > +		&& (code[3] == ',' || code[3] == '\0' || code[3] == ' '))
> 
> This doesn't allow other whitespace.  Maybe it's cleaner written as e.g.
> 
> 	&& codes - code == 3

... and that doesn't handle trailing whitespace.  Ugh.


Segher

Re: [PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

[Jakub Jelinek]

On Wed, Apr 08, 2015 at 05:16:08PM -0500, Segher Boessenkool wrote:
> On Wed, Apr 08, 2015 at 05:12:07PM -0500, Segher Boessenkool wrote:
> > On Wed, Apr 08, 2015 at 11:00:59PM +0200, Jakub Jelinek wrote:
> > > +    case MATCH_CODE:
> > > +      if (*XSTR (exp, 1) == '\0')
> > > +	{
> > > +	  const char *code, *codes = XSTR (exp, 0);
> > > +	  int ret = 0;
> > > +	  while ((code = scan_comma_elt (&codes)) != 0)
> > > +	    if (strncmp (code, "reg", 3) == 0
> > > +		&& (code[3] == ',' || code[3] == '\0' || code[3] == ' '))
> > 
> > This doesn't allow other whitespace.  Maybe it's cleaner written as e.g.
> > 
> > 	&& codes - code == 3
> 
> ... and that doesn't handle trailing whitespace.  Ugh.

Yeah.  Guess I should use
	&& (code[3] == ',' || code[3] == '\0' || ISSPACE (code[3]))
instead then.

	Jakub

Re: [PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

[Jakub Jelinek]

On Thu, Apr 09, 2015 at 12:20:40AM +0200, Jakub Jelinek wrote:
> On Wed, Apr 08, 2015 at 05:16:08PM -0500, Segher Boessenkool wrote:
> > On Wed, Apr 08, 2015 at 05:12:07PM -0500, Segher Boessenkool wrote:
> > > On Wed, Apr 08, 2015 at 11:00:59PM +0200, Jakub Jelinek wrote:
> > > > +    case MATCH_CODE:
> > > > +      if (*XSTR (exp, 1) == '\0')
> > > > +	{
> > > > +	  const char *code, *codes = XSTR (exp, 0);
> > > > +	  int ret = 0;
> > > > +	  while ((code = scan_comma_elt (&codes)) != 0)
> > > > +	    if (strncmp (code, "reg", 3) == 0
> > > > +		&& (code[3] == ',' || code[3] == '\0' || code[3] == ' '))
> > > 
> > > This doesn't allow other whitespace.  Maybe it's cleaner written as e.g.
> > > 
> > > 	&& codes - code == 3
> > 
> > ... and that doesn't handle trailing whitespace.  Ugh.
> 
> Yeah.  Guess I should use
> 	&& (code[3] == ',' || code[3] == '\0' || ISSPACE (code[3]))
> instead then.

Or another option is just to follow what genpreds.c uses earlier:
          || (!strstr (XSTR (exp, 0), "const_int")
              && !strstr (XSTR (exp, 0), "const_double")))
if (strstr (XSTR (exp, 0), "reg"))
  ret |= 1;
if (strstr (XSTR (exp, 0), "mem"))
  ret |= 2;
would match besides reg/subreg or mem also define_register_constraint
or define_memory_constraint, but those surely won't appear in sane
constraints.md (and even if they would, all we need is conservative test).

	Jakub

Re: [PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

[Jakub Jelinek]

On Wed, Apr 08, 2015 at 11:00:59PM +0200, Jakub Jelinek wrote:
> On x86_64/i686 this only detects constraint G as not allowing reg nor mem
> (it is match_code const_double), and V (plus < and >, but those are
> hardcoded in stmt.c already) that it allows mem but not reg.
> On aarch64, in the first category it detects several constraints.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

FYI, Alan Lawrence has successfully bootstrapped/regtested it also on
aarch64-linux and arm hf.

	Jakub

Re: [PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

[Richard Sandiford]

Jakub Jelinek <jakub@redhat.com> writes:
> Hi!
>
> Right now, stmt.c on constraints not hardcoded in it, and not
> define_{register,address,memory}_constraint just assumes the
> constraint might allow both reg and mem.  Unfortunately, on some
> constraints which clearly can't allow either of those leads to errors
> at -O0, because the expander doesn't try so hard to expand it as
> EXPAND_INITIALIZER.
>
> The following patch is an attempt to handle at least the easy cases
> - define_constraint like:
> (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)")))
> where the match_code clearly proves that it never can match any REG/SUBREG,
> nor MEM, by teaching genpreds.c to emit an extra inline function that
> stmt.c can in process_{output,input}_constraint use for the unknown
> constraints.
>
> On x86_64/i686 this only detects constraint G as not allowing reg nor mem
> (it is match_code const_double), and V (plus < and >, but those are
> hardcoded in stmt.c already) that it allows mem but not reg.
> On aarch64, in the first category it detects several constraints.

Thanks for doing this.

How about reusing compute_predicate_codes from gensupport.c?  It'd be
good to have a single function that understands how to get codes from
both predicate and constraint expressions rather than have slightly
different logic for the two.

Richard

Re: [PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

[James Greenhalgh]

On Wed, Apr 08, 2015 at 11:00:59PM +0200, Jakub Jelinek wrote:
> Hi!
> 
> Right now, stmt.c on constraints not hardcoded in it, and not
> define_{register,address,memory}_constraint just assumes the
> constraint might allow both reg and mem.  Unfortunately, on some
> constraints which clearly can't allow either of those leads to errors
> at -O0, because the expander doesn't try so hard to expand it as
> EXPAND_INITIALIZER.
> 
> The following patch is an attempt to handle at least the easy cases
> - define_constraint like:
> (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)")))
> where the match_code clearly proves that it never can match any REG/SUBREG,
> nor MEM, by teaching genpreds.c to emit an extra inline function that
> stmt.c can in process_{output,input}_constraint use for the unknown
> constraints.
> 
> On x86_64/i686 this only detects constraint G as not allowing reg nor mem
> (it is match_code const_double), and V (plus < and >, but those are
> hardcoded in stmt.c already) that it allows mem but not reg.
> On aarch64, in the first category it detects several constraints.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2015-04-08  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/65689
> 	* genpreds.c (struct constraint_data): Add maybe_allows_reg and
> 	maybe_allows_mem bitfields.
> 	(maybe_allows_none_start, maybe_allows_none_end,
> 	maybe_allows_reg_start, maybe_allows_reg_end, maybe_allows_mem_start,
> 	maybe_allows_mem_end): New variables.
> 	(compute_maybe_allows): New function.
> 	(add_constraint): Use it to initialize maybe_allows_reg and
> 	maybe_allows_mem fields.
> 	(choose_enum_order): Sort the non-is_register/is_const_int/is_memory/
> 	is_address constraints such that those that allow neither mem nor
> 	reg come first, then those that only allow reg but not mem, then
> 	those that only allow mem but not reg, then the rest.
> 	(write_allows_reg_mem_function): New function.
> 	(write_tm_preds_h): Call it.
> 	* stmt.c (parse_output_constraint, parse_input_constraint): Use
> 	the generated insn_extra_constraint_allows_reg_mem function
> 	instead of always setting *allows_reg = true; *allows_mem = true;
> 	for unknown extra constraints.

Hi Jakub,

This applies clean to gcc-5-branch. I've bootstrapped and tested it on
x86_64-none-linux-gnu, aarch64-none-linux-gnu and arm-none-linux-gnueabihf
with no problems.

Is this OK to commit to gcc-5-branch so I can close out PR 65689?

Thanks,
James

Re: [PATCH] Be less conservative in process_{output,input}_constraints (PR target/65689)

[Jakub Jelinek]

On Tue, Jan 12, 2016 at 12:30:20PM +0000, James Greenhalgh wrote:
> > 2015-04-08  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR target/65689
> > 	* genpreds.c (struct constraint_data): Add maybe_allows_reg and
> > 	maybe_allows_mem bitfields.
> > 	(maybe_allows_none_start, maybe_allows_none_end,
> > 	maybe_allows_reg_start, maybe_allows_reg_end, maybe_allows_mem_start,
> > 	maybe_allows_mem_end): New variables.
> > 	(compute_maybe_allows): New function.
> > 	(add_constraint): Use it to initialize maybe_allows_reg and
> > 	maybe_allows_mem fields.
> > 	(choose_enum_order): Sort the non-is_register/is_const_int/is_memory/
> > 	is_address constraints such that those that allow neither mem nor
> > 	reg come first, then those that only allow reg but not mem, then
> > 	those that only allow mem but not reg, then the rest.
> > 	(write_allows_reg_mem_function): New function.
> > 	(write_tm_preds_h): Call it.
> > 	* stmt.c (parse_output_constraint, parse_input_constraint): Use
> > 	the generated insn_extra_constraint_allows_reg_mem function
> > 	instead of always setting *allows_reg = true; *allows_mem = true;
> > 	for unknown extra constraints.
> 
> Hi Jakub,
> 
> This applies clean to gcc-5-branch. I've bootstrapped and tested it on
> x86_64-none-linux-gnu, aarch64-none-linux-gnu and arm-none-linux-gnueabihf
> with no problems.
> 
> Is this OK to commit to gcc-5-branch so I can close out PR 65689?

Ok, thanks.

	Jakub