[PING] genattrab.c generate switch

[PING] genattrab.c generate switch

[Jesper Broge Jørgensen]

Ping patch:

https://gcc.gnu.org/ml/gcc-patches/2016-01/msg00784.html

thanks

Re: [PING] genattrab.c generate switch

[Jeff Law]

On 01/18/2016 07:09 AM, Jesper Broge Jørgensen wrote:
> Ping patch:
>
> https://gcc.gnu.org/ml/gcc-patches/2016-01/msg00784.html
I'd put it in my gcc-7 queue.  But if Richard, Bernd, Richi or someone 
else wants to work though the changes as a bugfix for bootstrapping on 
platforms with crippled compilers, I won't object.

jeff

Re: [PING] genattrab.c generate switch

[Richard Biener]

On Mon, Jan 18, 2016 at 7:48 PM, Jeff Law <law@redhat.com> wrote:
> On 01/18/2016 07:09 AM, Jesper Broge Jørgensen wrote:
>>
>> Ping patch:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2016-01/msg00784.html
>
> I'd put it in my gcc-7 queue.  But if Richard, Bernd, Richi or someone else
> wants to work though the changes as a bugfix for bootstrapping on platforms
> with crippled compilers, I won't object.

I'd take it as a bugfix but the patch still needs review.

Richard.

> jeff

Re: [PING] genattrab.c generate switch

[Jesper Broge Jørgensen]

On 19/01/16 10:44, Richard Biener wrote:
> On Mon, Jan 18, 2016 at 7:48 PM, Jeff Law <law@redhat.com> wrote:
>> On 01/18/2016 07:09 AM, Jesper Broge Jørgensen wrote:
>>> Ping patch:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2016-01/msg00784.html
>> I'd put it in my gcc-7 queue.  But if Richard, Bernd, Richi or someone else
>> wants to work though the changes as a bugfix for bootstrapping on platforms
>> with crippled compilers, I won't object.
> I'd take it as a bugfix but the patch still needs review.
>
> Richard.
>
>> jeff
Here is the reformatted patch:


gcc/ChangeLog:

2016-01-19  Jesper Broge Jørgensen  <jesperbroge@gmail.com>

     * genattrtab.c (check_attr_set_switch): New function
     (write_attr_set): Write a switch instead of if condition, if possible


diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index 2caf8f6..8e7f9e6 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -4113,6 +4113,103 @@ eliminate_known_true (rtx known_true, rtx exp, 
int insn_code, int insn_index)
    return exp;
  }

+/* Check if exp contains a series of IOR conditions on the same attr_name.
+   If it does it can be turned into a switch statement and returns true.
+   If write_cases is true it will write the cases of the switch to 
outf.  */
+
+static int
+check_attr_set_switch (FILE *outf, rtx exp, unsigned int attrs_cached,
+               int write_cases, int indent)
+{
+  if (GET_CODE (exp) != IOR)
+    return 0;
+  if (GET_CODE (XEXP (exp, 0)) != EQ_ATTR)
+    return 0;
+
+  rtx next = exp;
+  int ior_depth = 0;
+  int is_first = 1;
+
+  const char *attr_name_cmp = XSTR (XEXP (exp, 0), 0);
+
+  while (1)
+    {
+      rtx op1 = XEXP (next, 0);
+      rtx op2 = XEXP (next, 1);
+
+      if (GET_CODE (op1) != EQ_ATTR)
+    return 0;
+
+      const char *attr_name = XSTR (op1, 0);
+      const char *cmp_val = XSTR (op1, 1);
+
+      /* pointer compare is enough.  */
+      if (attr_name_cmp != attr_name)
+    return 0;
+
+      if (write_cases)
+    {
+      struct attr_desc *attr = find_attr (&attr_name, 0);
+      gcc_assert (attr);
+      if (is_first)
+        {
+          fprintf (outf, "(");
+          is_first = 0;
+          int i;
+          for (i = 0; i < cached_attr_count; i++)
+        if (attr->name == cached_attrs[i])
+          break;
+
+          if (i < cached_attr_count && (attrs_cached & (1U << i)) != 0)
+        fprintf (outf, "cached_%s", attr->name);
+          else if (i < cached_attr_count &&
+               (attrs_to_cache & (1U << i)) != 0)
+        fprintf (outf, "(cached_%s = get_attr_%s (insn))", attr->name,
+             attr->name);
+          else
+        fprintf (outf, "get_attr_%s (insn)", attr->name);
+          fprintf (outf, ")\n");
+          write_indent (outf, indent);
+          fprintf (outf, "{\n");
+        }
+      write_indent (outf, indent);
+      fprintf (outf, "case ");
+      write_attr_valueq (outf, attr, cmp_val);
+      fprintf (outf, ":\n");
+    }
+
+      const int code = GET_CODE (op2);
+      if (code != IOR)
+    {
+      if (code == EQ_ATTR)
+        {
+          const char *attr_name = XSTR (op2, 0);
+          const char *cmp_val = XSTR (op2, 1);
+
+          if (attr_name == alternative_name)
+        return 0;
+
+          struct attr_desc *attr = find_attr (&attr_name, 0);
+          gcc_assert (attr);
+
+          if (attr->is_const)
+        return 0;
+          else if (write_cases)
+        {
+          write_indent (outf, indent);
+          fprintf (outf, "case ");
+          write_attr_valueq (outf, attr, cmp_val);
+          fprintf (outf, ":\n");
+        }
+        }
+      break;
+    }
+      next = op2;
+      ior_depth++;
+    }
+  return ior_depth > 2;
+}
+
  /* Write out a series of tests and assignment statements to perform 
tests and
     sets of an attribute value.  We are passed an indentation amount 
and prefix
     and suffix strings to write around each attribute value (e.g., "return"
@@ -4123,6 +4220,7 @@ write_attr_set (FILE *outf, struct attr_desc 
*attr, int indent, rtx value,
          const char *prefix, const char *suffix, rtx known_true,
          int insn_code, int insn_index, unsigned int attrs_cached)
  {
+  int n_switches = 0;
    if (GET_CODE (value) == COND)
      {
        /* Assume the default value will be the default of the COND 
unless we
@@ -4132,6 +4230,7 @@ write_attr_set (FILE *outf, struct attr_desc 
*attr, int indent, rtx value,
        rtx newexp;
        int first_if = 1;
        int i;
+      int is_switch = 0;

        if (cached_attr_count)
      {
@@ -4176,40 +4275,68 @@ write_attr_set (FILE *outf, struct attr_desc 
*attr, int indent, rtx value,
        if (inner_true == false_rtx)
          continue;

+      is_switch = check_attr_set_switch (outf, testexp, attrs_cached, 0,
+                         indent);
+
        attrs_cached_inside = attrs_cached;
        attrs_cached_after = attrs_cached;
        write_indent (outf, indent);
-      fprintf (outf, "%sif ", first_if ? "" : "else ");
-      first_if = 0;
-      write_test_expr (outf, testexp, attrs_cached,
-               (FLG_AFTER | FLG_INSIDE | FLG_OUTSIDE_AND));
-      attrs_cached = attrs_cached_after;
-      fprintf (outf, "\n");
-      write_indent (outf, indent + 2);
-      fprintf (outf, "{\n");
+      if (is_switch)
+        {
+          fprintf (outf, "switch ");
+          n_switches++;
+          first_if = 1;
+          check_attr_set_switch (outf, testexp, attrs_cached, 1, indent);
+          indent += 4;
+        }
+      else
+        {
+          fprintf (outf, "%sif ", first_if ? "" : "else ");
+          first_if = 0;
+          write_test_expr (outf, testexp, attrs_cached,
+                   (FLG_AFTER | FLG_INSIDE | FLG_OUTSIDE_AND));
+          attrs_cached = attrs_cached_after;
+          fprintf (outf, "\n");
+        }
+      if (!is_switch)
+        {
+          write_indent (outf, indent + 2);
+          fprintf (outf, "{\n");
+        }
+

        write_attr_set (outf, attr, indent + 4,
                XVECEXP (value, 0, i + 1), prefix, suffix,
                inner_true, insn_code, insn_index,
                attrs_cached_inside);
        write_indent (outf, indent + 2);
-      fprintf (outf, "}\n");
+      if (is_switch)
+        {
+          fprintf (outf, "break;\n");
+          write_indent (outf, indent);
+          fprintf (outf, "default:\n");
+          indent += 4;
+        }
+      else
+        fprintf (outf, "}\n");
        our_known_true = newexp;
      }

-      if (! first_if)
+      if (! first_if && ! is_switch)
      {
        write_indent (outf, indent);
        fprintf (outf, "else\n");
        write_indent (outf, indent + 2);
        fprintf (outf, "{\n");
      }
+      else if (is_switch)
+    write_indent (outf, indent);

-      write_attr_set (outf, attr, first_if ? indent : indent + 4, 
default_val,
+      write_attr_set (outf, attr, (first_if || is_switch) ? indent : 
indent + 4, default_val,
                prefix, suffix, our_known_true, insn_code, insn_index,
                attrs_cached);

-      if (! first_if)
+      if (! first_if && ! is_switch)
      {
        write_indent (outf, indent + 2);
        fprintf (outf, "}\n");
@@ -4222,6 +4349,12 @@ write_attr_set (FILE *outf, struct attr_desc 
*attr, int indent, rtx value,
        write_attr_value (outf, attr, value);
        fprintf (outf, "%s\n", suffix);
      }
+  while (n_switches--)
+    {
+      indent -= 2;
+      write_indent (outf, indent);
+      fprintf (outf, "}\n");
+    }
  }

  /* Write a series of case statements for every instruction in list IE.

Re: [PING] genattrab.c generate switch

[Jesper Broge Jørgensen]

On 19/01/16 10:44, Richard Biener wrote:
> On Mon, Jan 18, 2016 at 7:48 PM, Jeff Law <law@redhat.com> wrote:
>> On 01/18/2016 07:09 AM, Jesper Broge Jørgensen wrote:
>>> Ping patch:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2016-01/msg00784.html
>> I'd put it in my gcc-7 queue.  But if Richard, Bernd, Richi or someone else
>> wants to work though the changes as a bugfix for bootstrapping on platforms
>> with crippled compilers, I won't object.
> I'd take it as a bugfix but the patch still needs review.
>
> Richard.
>
>> jeff
I have finally received a confirmation from fsf that they received my 
copyright assignment so i am now ready to have the patch reviewed.

Re: [PING] genattrab.c generate switch

[Bernd Schmidt]

On 01/19/2016 12:47 PM, Jesper Broge Jørgensen wrote:
> Here is the reformatted patch:

This will probably have to wait until stage1.

> +      const int code = GET_CODE (op2);
> +      if (code != IOR)
> +    {
> +      if (code == EQ_ATTR)

All the formatting still looks completely mangled. This was probably 
done by your mailer. Please try attaching the diff as text/plain.


Bernd

Re: [PING] genattrab.c generate switch

[Jesper Broge Jørgensen]

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


On 18/02/16 13:22, Bernd Schmidt wrote:
> On 01/19/2016 12:47 PM, Jesper Broge Jørgensen wrote:
>> Here is the reformatted patch:
>
> This will probably have to wait until stage1.
>
>> +      const int code = GET_CODE (op2);
>> +      if (code != IOR)
>> +    {
>> +      if (code == EQ_ATTR)
>
> All the formatting still looks completely mangled. This was probably 
> done by your mailer. Please try attaching the diff as text/plain.
>
>
> Bernd
>
Here it is as an attatchment. I set deliveryformat to "Plain Text Only" 
in thunderbird.

Please let me know if anything else needs to be changed.

[-- Attachment #2: gcc_patch_reformatted.patch --]
[-- Type: text/plain, Size: 6501 bytes --]

gcc/ChangeLog:

2016-01-19  Jesper Broge Jørgensen  <jesperbroge@gmail.com>

	* genattrtab.c (check_attr_set_switch): New function
	(write_attr_set): Write a switch instead of if condition if possible


diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index 2caf8f6..8e7f9e6 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -4113,6 +4113,103 @@ eliminate_known_true (rtx known_true, rtx exp, int insn_code, int insn_index)
   return exp;
 }
 
+/* Check if exp contains a series of IOR conditions on the same attr_name.
+   If it does it can be turned into a switch statement and returns true.
+   If write_cases is true it will write the cases of the switch to outf.  */
+
+static int
+check_attr_set_switch (FILE *outf, rtx exp, unsigned int attrs_cached,
+		       int write_cases, int indent)
+{
+  if (GET_CODE (exp) != IOR)
+    return 0;
+  if (GET_CODE (XEXP (exp, 0)) != EQ_ATTR)
+    return 0;
+
+  rtx next = exp;
+  int ior_depth = 0;
+  int is_first = 1;
+
+  const char *attr_name_cmp = XSTR (XEXP (exp, 0), 0);
+
+  while (1)
+    {
+      rtx op1 = XEXP (next, 0);
+      rtx op2 = XEXP (next, 1);
+
+      if (GET_CODE (op1) != EQ_ATTR)
+	return 0;
+
+      const char *attr_name = XSTR (op1, 0);
+      const char *cmp_val = XSTR (op1, 1);
+
+      /* pointer compare is enough.  */
+      if (attr_name_cmp != attr_name)
+	return 0;
+
+      if (write_cases)
+	{
+	  struct attr_desc *attr = find_attr (&attr_name, 0);
+	  gcc_assert (attr);
+	  if (is_first)
+	    {
+	      fprintf (outf, "(");
+	      is_first = 0;
+	      int i;
+	      for (i = 0; i < cached_attr_count; i++)
+		if (attr->name == cached_attrs[i])
+		  break;
+
+	      if (i < cached_attr_count && (attrs_cached & (1U << i)) != 0)
+		fprintf (outf, "cached_%s", attr->name);
+	      else if (i < cached_attr_count &&
+		       (attrs_to_cache & (1U << i)) != 0)
+		fprintf (outf, "(cached_%s = get_attr_%s (insn))", attr->name,
+			 attr->name);
+	      else
+		fprintf (outf, "get_attr_%s (insn)", attr->name);
+	      fprintf (outf, ")\n");
+	      write_indent (outf, indent);
+	      fprintf (outf, "{\n");
+	    }
+	  write_indent (outf, indent);
+	  fprintf (outf, "case ");
+	  write_attr_valueq (outf, attr, cmp_val);
+	  fprintf (outf, ":\n");
+	}
+
+      const int code = GET_CODE (op2);
+      if (code != IOR)
+	{
+	  if (code == EQ_ATTR)
+	    {
+	      const char *attr_name = XSTR (op2, 0);
+	      const char *cmp_val = XSTR (op2, 1);
+
+	      if (attr_name == alternative_name)
+		return 0;
+
+	      struct attr_desc *attr = find_attr (&attr_name, 0);
+	      gcc_assert (attr);
+
+	      if (attr->is_const)
+		return 0;
+	      else if (write_cases)
+		{
+		  write_indent (outf, indent);
+		  fprintf (outf, "case ");
+		  write_attr_valueq (outf, attr, cmp_val);
+		  fprintf (outf, ":\n");
+		}
+	    }
+	  break;
+	}
+      next = op2;
+      ior_depth++;
+    }
+  return ior_depth > 2;
+}
+
 /* Write out a series of tests and assignment statements to perform tests and
    sets of an attribute value.  We are passed an indentation amount and prefix
    and suffix strings to write around each attribute value (e.g., "return"
@@ -4123,6 +4220,7 @@ write_attr_set (FILE *outf, struct attr_desc *attr, int indent, rtx value,
 		const char *prefix, const char *suffix, rtx known_true,
 		int insn_code, int insn_index, unsigned int attrs_cached)
 {
+  int n_switches = 0;
   if (GET_CODE (value) == COND)
     {
       /* Assume the default value will be the default of the COND unless we
@@ -4132,6 +4230,7 @@ write_attr_set (FILE *outf, struct attr_desc *attr, int indent, rtx value,
       rtx newexp;
       int first_if = 1;
       int i;
+      int is_switch = 0;
 
       if (cached_attr_count)
 	{
@@ -4176,40 +4275,68 @@ write_attr_set (FILE *outf, struct attr_desc *attr, int indent, rtx value,
 	  if (inner_true == false_rtx)
 	    continue;
 
+	  is_switch = check_attr_set_switch (outf, testexp, attrs_cached, 0,
+					     indent);
+
 	  attrs_cached_inside = attrs_cached;
 	  attrs_cached_after = attrs_cached;
 	  write_indent (outf, indent);
-	  fprintf (outf, "%sif ", first_if ? "" : "else ");
-	  first_if = 0;
-	  write_test_expr (outf, testexp, attrs_cached,
-			   (FLG_AFTER | FLG_INSIDE | FLG_OUTSIDE_AND));
-	  attrs_cached = attrs_cached_after;
-	  fprintf (outf, "\n");
-	  write_indent (outf, indent + 2);
-	  fprintf (outf, "{\n");
+	  if (is_switch)
+	    {
+	      fprintf (outf, "switch ");
+	      n_switches++;
+	      first_if = 1;
+	      check_attr_set_switch (outf, testexp, attrs_cached, 1, indent);
+	      indent += 4;
+	    }
+	  else
+	    {
+	      fprintf (outf, "%sif ", first_if ? "" : "else ");
+	      first_if = 0;
+	      write_test_expr (outf, testexp, attrs_cached,
+			       (FLG_AFTER | FLG_INSIDE | FLG_OUTSIDE_AND));
+	      attrs_cached = attrs_cached_after;
+	      fprintf (outf, "\n");
+	    }
+	  if (!is_switch)
+	    {
+	      write_indent (outf, indent + 2);
+	      fprintf (outf, "{\n");
+	    }
+
 
 	  write_attr_set (outf, attr, indent + 4,
 			  XVECEXP (value, 0, i + 1), prefix, suffix,
 			  inner_true, insn_code, insn_index,
 			  attrs_cached_inside);
 	  write_indent (outf, indent + 2);
-	  fprintf (outf, "}\n");
+	  if (is_switch)
+	    {
+	      fprintf (outf, "break;\n");
+	      write_indent (outf, indent);
+	      fprintf (outf, "default:\n");
+	      indent += 4;
+	    }
+	  else
+	    fprintf (outf, "}\n");
 	  our_known_true = newexp;
 	}
 
-      if (! first_if)
+      if (! first_if && ! is_switch)
 	{
 	  write_indent (outf, indent);
 	  fprintf (outf, "else\n");
 	  write_indent (outf, indent + 2);
 	  fprintf (outf, "{\n");
 	}
+      else if (is_switch)
+	write_indent (outf, indent);
 
-      write_attr_set (outf, attr, first_if ? indent : indent + 4, default_val,
+      write_attr_set (outf, attr, (first_if || is_switch) ? indent : indent + 4, default_val,
 		      prefix, suffix, our_known_true, insn_code, insn_index,
 		      attrs_cached);
 
-      if (! first_if)
+      if (! first_if && ! is_switch)
 	{
 	  write_indent (outf, indent + 2);
 	  fprintf (outf, "}\n");
@@ -4222,6 +4349,12 @@ write_attr_set (FILE *outf, struct attr_desc *attr, int indent, rtx value,
       write_attr_value (outf, attr, value);
       fprintf (outf, "%s\n", suffix);
     }
+  while (n_switches--)
+    {
+      indent -= 2;
+      write_indent (outf, indent);
+      fprintf (outf, "}\n");
+    }
 }
 
 /* Write a series of case statements for every instruction in list IE.

Re: [PING] genattrab.c generate switch

[Jesper Broge Jørgensen]

On 18/02/16 13:22, Bernd Schmidt wrote:
> On 01/19/2016 12:47 PM, Jesper Broge Jørgensen wrote:
>> Here is the reformatted patch:
>
> This will probably have to wait until stage1.
>
>> +      const int code = GET_CODE (op2);
>> +      if (code != IOR)
>> +    {
>> +      if (code == EQ_ATTR)
>
> All the formatting still looks completely mangled. This was probably 
> done by your mailer. Please try attaching the diff as text/plain.
>
>
> Bernd
>
Hi i send the patch back as an attatchment as requested about two weeks 
ago (https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01256.html) but i 
have not received any response.

If it has to wait for stage 1 are there anything else i can do for the 
patch untill then?

Re: [PING] genattrab.c generate switch

[Patrick Palka]

On Thu, Mar 3, 2016 at 4:29 PM, Jesper Broge Jørgensen
<jesperbroge@gmail.com> wrote:
>
> On 18/02/16 13:22, Bernd Schmidt wrote:
>>
>> On 01/19/2016 12:47 PM, Jesper Broge Jørgensen wrote:
>>>
>>> Here is the reformatted patch:
>>
>>
>> This will probably have to wait until stage1.
>>
>>> +      const int code = GET_CODE (op2);
>>> +      if (code != IOR)
>>> +    {
>>> +      if (code == EQ_ATTR)
>>
>>
>> All the formatting still looks completely mangled. This was probably done
>> by your mailer. Please try attaching the diff as text/plain.
>>
>>
>> Bernd
>>
> Hi i send the patch back as an attatchment as requested about two weeks ago
> (https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01256.html) but i have not
> received any response.
>
> If it has to wait for stage 1 are there anything else i can do for the patch
> untill then?

I still suggest to try making write_test_expr() avoid emitting
redundant parentheses for chains of || or &&, which would fix the
original issue all the same.  Previously you claimed that such a
change would not be simpler than your current patch, but I gave it a
quick try and ended up with a much smaller patch:

 gcc/genattrtab.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

Re: [PING] genattrab.c generate switch

[David Malcolm]

On Thu, 2016-03-03 at 17:36 -0500, Patrick Palka wrote:
> On Thu, Mar 3, 2016 at 4:29 PM, Jesper Broge Jørgensen
> <jesperbroge@gmail.com> wrote:
> > 
> > On 18/02/16 13:22, Bernd Schmidt wrote:
> > > 
> > > On 01/19/2016 12:47 PM, Jesper Broge Jørgensen wrote:
> > > > 
> > > > Here is the reformatted patch:
> > > 
> > > 
> > > This will probably have to wait until stage1.
> > > 
> > > > +      const int code = GET_CODE (op2);
> > > > +      if (code != IOR)
> > > > +    {
> > > > +      if (code == EQ_ATTR)
> > > 
> > > 
> > > All the formatting still looks completely mangled. This was
> > > probably done
> > > by your mailer. Please try attaching the diff as text/plain.
> > > 
> > > 
> > > Bernd
> > > 
> > Hi i send the patch back as an attatchment as requested about two
> > weeks ago
> > (https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01256.html) but i
> > have not
> > received any response.
> > 
> > If it has to wait for stage 1 are there anything else i can do for
> > the patch
> > untill then?
> 
> I still suggest to try making write_test_expr() avoid emitting
> redundant parentheses for chains of || or &&, which would fix the
> original issue all the same.  Previously you claimed that such a
> change would not be simpler than your current patch, but I gave it a
> quick try and ended up with a much smaller patch:
> 
>  gcc/genattrtab.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)

Patrick, did you forget to attach the patch?  I see the diffstat, but
no patch.

Re: [PING] genattrab.c generate switch

[Patrick Palka]

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

On Fri, 4 Mar 2016, David Malcolm wrote:

> On Thu, 2016-03-03 at 17:36 -0500, Patrick Palka wrote:
>> On Thu, Mar 3, 2016 at 4:29 PM, Jesper Broge Jørgensen
>> <jesperbroge@gmail.com> wrote:
>>>
>>> On 18/02/16 13:22, Bernd Schmidt wrote:
>>>>
>>>> On 01/19/2016 12:47 PM, Jesper Broge Jørgensen wrote:
>>>>>
>>>>> Here is the reformatted patch:
>>>>
>>>>
>>>> This will probably have to wait until stage1.
>>>>
>>>>> +      const int code = GET_CODE (op2);
>>>>> +      if (code != IOR)
>>>>> +    {
>>>>> +      if (code == EQ_ATTR)
>>>>
>>>>
>>>> All the formatting still looks completely mangled. This was
>>>> probably done
>>>> by your mailer. Please try attaching the diff as text/plain.
>>>>
>>>>
>>>> Bernd
>>>>
>>> Hi i send the patch back as an attatchment as requested about two
>>> weeks ago
>>> (https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01256.html) but i
>>> have not
>>> received any response.
>>>
>>> If it has to wait for stage 1 are there anything else i can do for
>>> the patch
>>> untill then?
>>
>> I still suggest to try making write_test_expr() avoid emitting
>> redundant parentheses for chains of || or &&, which would fix the
>> original issue all the same.  Previously you claimed that such a
>> change would not be simpler than your current patch, but I gave it a
>> quick try and ended up with a much smaller patch:
>>
>>  gcc/genattrtab.c | 26 +++++++++++++++++++-------
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> Patrick, did you forget to attach the patch?  I see the diffstat, but
> no patch.
>

Here it is:

-- >8 --

Subject: [PATCH] Reduce nesting of parentheses in conditionals generated by
  genattrtab

gcc/ChangeLog:

 	* genattrtab.c (write_test_expr): New parameter EMIT_PARENS
 	which defaults to true.  Emit an outer pair of parentheses only if
 	EMIT_PARENS.  When continuing a chain of && or ||, don't emit
 	parentheses for the right-hand operand.
---
  gcc/genattrtab.c | 26 +++++++++++++++++++-------
  1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index b64d8b9..e2ccf1f 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -3432,16 +3432,16 @@ find_attrs_to_cache (rtx exp, bool create)
  #define FLG_OUTSIDE_AND		8

  static unsigned int
-write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags)
+write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags,
+		 bool emit_parens = true)
  {
    int comparison_operator = 0;
    RTX_CODE code;
    struct attr_desc *attr;

-  /* In order not to worry about operator precedence, surround our part of
-     the expression with parentheses.  */
+  if (emit_parens)
+    fprintf (outf, "(");

-  fprintf (outf, "(");
    code = GET_CODE (exp);
    switch (code)
      {
@@ -3575,8 +3575,18 @@ write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags)
  	      || GET_CODE (XEXP (exp, 1)) == EQ_ATTR
  	      || (GET_CODE (XEXP (exp, 1)) == NOT
  		  && GET_CODE (XEXP (XEXP (exp, 1), 0)) == EQ_ATTR)))
-	attrs_cached
-	  = write_test_expr (outf, XEXP (exp, 1), attrs_cached, flags);
+	{
+	  bool need_parens = true;
+
+	  /* No need to emit parentheses around the right-hand operand if we are
+	     continuing a chain of && or ||.  */
+	  if (GET_CODE (XEXP (exp, 1)) == code)
+	    need_parens = false;
+
+	  attrs_cached
+	    = write_test_expr (outf, XEXP (exp, 1), attrs_cached, flags,
+			       need_parens);
+	}
        else
  	write_test_expr (outf, XEXP (exp, 1), attrs_cached,
  			 flags | comparison_operator);
@@ -3794,7 +3804,9 @@ write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags)
  	     GET_RTX_NAME (code));
      }

-  fprintf (outf, ")");
+  if (emit_parens)
+    fprintf (outf, ")");
+
    return attrs_cached;
  }

-- 
2.8.0.rc0.11.g9bfbc33

Re: [PING] genattrab.c generate switch

[Bernd Schmidt]

On 03/04/2016 03:27 PM, Patrick Palka wrote:
>>> I still suggest to try making write_test_expr() avoid emitting
>>> redundant parentheses for chains of || or &&, which would fix the
>>> original issue all the same.  Previously you claimed that such a
>>> change would not be simpler than your current patch, but I gave it a
>>> quick try and ended up with a much smaller patch:

This looks like a reasonable stopgap if a release manager thinks this is 
important enough to fix for gcc-6. Some comments below for that case. 
Longer term I'm not sure - in theory maybe the switch would allow us to 
generate better code, but I tried it and code size actually seems to go 
up (could be jump tables however). I also noticed that in the version 
with the switch we still have cases of

switch (cached_type)
{
  ....
  default:
    switch (cached_type)
    {
    }
}

so that might be a point where the patch could be improved to see if we 
can get better code generation by collapsing these switches. Might also 
be worth trying to optimize this pattern in gcc.

>   static unsigned int
> -write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int
> flags)
> +write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int
> flags,
> +         bool emit_parens = true)

Indentation looks wrong, but could be mailer damage. You should also 
update the function comment.

> +      attrs_cached
> +        = write_test_expr (outf, XEXP (exp, 1), attrs_cached, flags,
> +                   need_parens);

Same here, watch indentation.


Bernd

Re: [PING] genattrab.c generate switch

[Jakub Jelinek]

On Fri, Mar 04, 2016 at 04:13:41PM +0100, Bernd Schmidt wrote:
> On 03/04/2016 03:27 PM, Patrick Palka wrote:
> >>>I still suggest to try making write_test_expr() avoid emitting
> >>>redundant parentheses for chains of || or &&, which would fix the
> >>>original issue all the same.  Previously you claimed that such a
> >>>change would not be simpler than your current patch, but I gave it a
> >>>quick try and ended up with a much smaller patch:
> 
> This looks like a reasonable stopgap if a release manager thinks this is
> important enough to fix for gcc-6. Some comments below for that case. Longer

I think it is important for gcc-6, because one compiler for
weirdo reasons imposes unreasonably small restrictions on the () nesting
and some people use it as stage1 compiler.

So, with the formatting nits fixed, if it got appropriately tested, I think
we want it for stage4.

	Jakub

Re: [PING] genattrab.c generate switch

[Jesper Broge Jørgensen]

On 04/03/16 16:13, Bernd Schmidt wrote:
> On 03/04/2016 03:27 PM, Patrick Palka wrote:
>>>> I still suggest to try making write_test_expr() avoid emitting
>>>> redundant parentheses for chains of || or &&, which would fix the
>>>> original issue all the same.  Previously you claimed that such a
>>>> change would not be simpler than your current patch, but I gave it a
>>>> quick try and ended up with a much smaller patch:
>
> This looks like a reasonable stopgap if a release manager thinks this 
> is important enough to fix for gcc-6. Some comments below for that 
> case. Longer term I'm not sure - in theory maybe the switch would 
> allow us to generate better code, but I tried it and code size 
> actually seems to go up (could be jump tables however). I also noticed 
> that in the version with the switch we still have cases of
>
> switch (cached_type)
> {
>  ....
>  default:
>    switch (cached_type)
>    {
>    }
> }
>
> so that might be a point where the patch could be improved to see if 
> we can get better code generation by collapsing these switches. Might 
> also be worth trying to optimize this pattern in gcc.
>
>
>
> Bernd
I can look into that if you deem it worth it, or would you rather just 
go with Patriks suppressed parenthesis?

Re: [PING] genattrab.c generate switch

[Patrick Palka]

On Fri, 4 Mar 2016, Jakub Jelinek wrote:

> On Fri, Mar 04, 2016 at 04:13:41PM +0100, Bernd Schmidt wrote:
> > On 03/04/2016 03:27 PM, Patrick Palka wrote:
> > >>>I still suggest to try making write_test_expr() avoid emitting
> > >>>redundant parentheses for chains of || or &&, which would fix the
> > >>>original issue all the same.  Previously you claimed that such a
> > >>>change would not be simpler than your current patch, but I gave it a
> > >>>quick try and ended up with a much smaller patch:
> > 
> > This looks like a reasonable stopgap if a release manager thinks this is
> > important enough to fix for gcc-6. Some comments below for that case. Longer
> 
> I think it is important for gcc-6, because one compiler for
> weirdo reasons imposes unreasonably small restrictions on the () nesting
> and some people use it as stage1 compiler.
> 
> So, with the formatting nits fixed, if it got appropriately tested, I think
> we want it for stage4.

I updated the function comment and made sure that the indentation is
correct.  Earlier I successfully bootstrapped + tested this patch on
x86_64-pc-linux-gnu, but I will do so again to make sure.  This patch
reduces the maximum parentheses nesting level in insn-attrtab.c on
x86_64 from about 31 to about 6.

OK to commit after testing?

-- >8 --

Subject: [PATCH] Reduce nesting of parentheses in conditionals generated by
 genattrtab

gcc/ChangeLog:

	* genattrtab.c (write_test_expr): New parameter EMIT_PARENS
	which defaults to true.  Emit an outer pair of parentheses only if
	EMIT_PARENS.  When continuing a chain of && or ||, don't emit
	parentheses for the right-hand operand.
---
 gcc/genattrtab.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index b64d8b9..5974f3e 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -3416,7 +3416,10 @@ find_attrs_to_cache (rtx exp, bool create)
 
 /* Given a piece of RTX, print a C expression to test its truth value to OUTF.
    We use AND and IOR both for logical and bit-wise operations, so
-   interpret them as logical unless they are inside a comparison expression.  */
+   interpret them as logical unless they are inside a comparison expression.
+
+   An outermost pair of parentheses is emitted around this C expression unless
+   EMIT_PARENS is false.  */
 
 /* Interpret AND/IOR as bit-wise operations instead of logical.  */
 #define FLG_BITWISE		1
@@ -3432,16 +3435,16 @@ find_attrs_to_cache (rtx exp, bool create)
 #define FLG_OUTSIDE_AND		8
 
 static unsigned int
-write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags)
+write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags,
+		 bool emit_parens = true)
 {
   int comparison_operator = 0;
   RTX_CODE code;
   struct attr_desc *attr;
 
-  /* In order not to worry about operator precedence, surround our part of
-     the expression with parentheses.  */
+  if (emit_parens)
+    fprintf (outf, "(");
 
-  fprintf (outf, "(");
   code = GET_CODE (exp);
   switch (code)
     {
@@ -3575,8 +3578,18 @@ write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags)
 	      || GET_CODE (XEXP (exp, 1)) == EQ_ATTR
 	      || (GET_CODE (XEXP (exp, 1)) == NOT
 		  && GET_CODE (XEXP (XEXP (exp, 1), 0)) == EQ_ATTR)))
-	attrs_cached
-	  = write_test_expr (outf, XEXP (exp, 1), attrs_cached, flags);
+	{
+	  bool need_parens = true;
+
+	  /* No need to emit parentheses around the right-hand operand if we are
+	     continuing a chain of && or ||.  */
+	  if (GET_CODE (XEXP (exp, 1)) == code)
+	    need_parens = false;
+
+	  attrs_cached
+	    = write_test_expr (outf, XEXP (exp, 1), attrs_cached, flags,
+			       need_parens);
+	}
       else
 	write_test_expr (outf, XEXP (exp, 1), attrs_cached,
 			 flags | comparison_operator);
@@ -3794,7 +3807,9 @@ write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags)
 	     GET_RTX_NAME (code));
     }
 
-  fprintf (outf, ")");
+  if (emit_parens)
+    fprintf (outf, ")");
+
   return attrs_cached;
 }
 
-- 
2.8.0.rc0.11.g9bfbc33

Re: [PING] genattrab.c generate switch

[Jakub Jelinek]

On Fri, Mar 04, 2016 at 11:34:06AM -0500, Patrick Palka wrote:
> 	* genattrtab.c (write_test_expr): New parameter EMIT_PARENS
> 	which defaults to true.  Emit an outer pair of parentheses only if
> 	EMIT_PARENS.  When continuing a chain of && or ||, don't emit
> 	parentheses for the right-hand operand.

Can you additionally to bootstrap/regtest on x86_64-linux compare
insn-attrtab.s (compiled with -g0 and optimizations on) if it is bitwise
identical without/with the patch (that should be the case, right),
and also try say cross-compiler to armv7hl-linux-gnueabi (where I believe
the nesting depth is significantly larger) and compare insn-attrtab.s
there as well (no need to build arm binutils, just configure cross-compiler
and let the build die after it builds cc1/cc1plus or even far before; all
we care is that insn-attrtab.s is bitwise the same)?

	Jakub

Re: [PING] genattrab.c generate switch

[Patrick Palka]

On Fri, Mar 4, 2016 at 11:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Mar 04, 2016 at 11:34:06AM -0500, Patrick Palka wrote:
>>       * genattrtab.c (write_test_expr): New parameter EMIT_PARENS
>>       which defaults to true.  Emit an outer pair of parentheses only if
>>       EMIT_PARENS.  When continuing a chain of && or ||, don't emit
>>       parentheses for the right-hand operand.
>
> Can you additionally to bootstrap/regtest on x86_64-linux compare
> insn-attrtab.s (compiled with -g0 and optimizations on) if it is bitwise
> identical without/with the patch (that should be the case, right),
> and also try say cross-compiler to armv7hl-linux-gnueabi (where I believe
> the nesting depth is significantly larger) and compare insn-attrtab.s
> there as well (no need to build arm binutils, just configure cross-compiler
> and let the build die after it builds cc1/cc1plus or even far before; all
> we care is that insn-attrtab.s is bitwise the same)?

I just quickly tested building the generated insn-attrtab.c with and
without the patch using my host gcc 5.3 compiler and the .s output is
not the same.

So the patch may be wrong, or the removal of parentheses is
functionally harmless but subtly affects code generation -- which is
plausible && and || naturally associate left-to-right but the explicit
parentheses emitted by write_test_expr effectively made chains of &&
or || to associate right-to-left.  This change in associativity could
be affecting the behavior of optimization passes, in theory.

Or the change could just be wrong, although the earlier successful
bootstrap + regtest suggests not.

Re: [PING] genattrab.c generate switch

[Bernd Schmidt]

On 03/04/2016 06:14 PM, Patrick Palka wrote:

> I just quickly tested building the generated insn-attrtab.c with and
> without the patch using my host gcc 5.3 compiler and the .s output is
> not the same.

Hmm, looking at the 003t.original dump it looks like there are 
differences in SAVE_EXPRs. Indeed we seem to generate different code for

int at;

int foo ()
{
   if (at == 2 || at == 4 || at == 7)
     return 1;
   return 0;
}

int bar ()
{
   if (at == 2 || (at == 4 || at == 7))
     return 1;
   return 0;
}

That's probably something we want to fix.


Bernd

Re: [PING] genattrab.c generate switch

[Bernd Schmidt]

On 03/04/2016 05:03 PM, Jesper Broge Jørgensen wrote:

> I can look into that if you deem it worth it, or would you rather just
> go with Patriks suppressed parenthesis?

For the moment (in stage4) we'll at most go with Patrick's patch. 
Whether we do anything beyond that depends on whether we can demonstrate 
a benefit - it might be worth investigating. Maybe more interesting than 
fixing genattrtab would be an optimization to identify switch patterns 
like the one I posted.


Bernd

Re: [PING] genattrab.c generate switch

[Bernd Schmidt]

On 03/04/2016 06:27 PM, Bernd Schmidt wrote:
> On 03/04/2016 06:14 PM, Patrick Palka wrote:
>
>> I just quickly tested building the generated insn-attrtab.c with and
>> without the patch using my host gcc 5.3 compiler and the .s output is
>> not the same.
>
> Hmm, looking at the 003t.original dump it looks like there are
> differences in SAVE_EXPRs. Indeed we seem to generate different code for
>
> int at;
>
> int foo ()
> {
>    if (at == 2 || at == 4 || at == 7)
>      return 1;
>    return 0;
> }
>
> int bar ()
> {
>    if (at == 2 || (at == 4 || at == 7))
>      return 1;
>    return 0;
> }

Ahh... it's not just different placement of SAVE_EXPRs, it's actually a 
case of TRUTH_ORIF_EXPR vs. TRUTH_OR_EXPR (the distinction is invisible 
in the dumps), the latter being created by fold_range_test. That's a bit 
of a broken optimization what with its inability to see more than two 
comparisons at a time... we convert one ORIF per function, but a 
different one.


Bernd

Re: [PING] genattrab.c generate switch

[Jakub Jelinek]

On Fri, Mar 04, 2016 at 06:49:58PM +0100, Bernd Schmidt wrote:
> On 03/04/2016 06:27 PM, Bernd Schmidt wrote:
> >On 03/04/2016 06:14 PM, Patrick Palka wrote:
> >
> >>I just quickly tested building the generated insn-attrtab.c with and
> >>without the patch using my host gcc 5.3 compiler and the .s output is
> >>not the same.
> >
> >Hmm, looking at the 003t.original dump it looks like there are
> >differences in SAVE_EXPRs. Indeed we seem to generate different code for
> >
> >int at;
> >
> >int foo ()
> >{
> >   if (at == 2 || at == 4 || at == 7)
> >     return 1;
> >   return 0;
> >}
> >
> >int bar ()
> >{
> >   if (at == 2 || (at == 4 || at == 7))
> >     return 1;
> >   return 0;
> >}
> 
> Ahh... it's not just different placement of SAVE_EXPRs, it's actually a case
> of TRUTH_ORIF_EXPR vs. TRUTH_OR_EXPR (the distinction is invisible in the
> dumps), the latter being created by fold_range_test. That's a bit of a
> broken optimization what with its inability to see more than two comparisons
> at a time... we convert one ORIF per function, but a different one.

I think we don't need to guarantee identical assembly, the reason I've
suggested that was if it passed, it would be much easier to verify.
Without that, I think it should be bootstrapped at least on one other
target.  Note the cases you remove the parens aren't just || and &&, but
most likely also | and & (at least there is some flag whether to print those
as && or &).  And there is code for the caching of the attributes where the
result is still usable, I believe the patch doesn't break that, but it
wouldn't hurt to verify that.

	Jakub

Re: [PING] genattrab.c generate switch

[Bernd Schmidt]

On 03/04/2016 06:56 PM, Jakub Jelinek wrote:
> I think we don't need to guarantee identical assembly, the reason I've
> suggested that was if it passed, it would be much easier to verify.
> Without that, I think it should be bootstrapped at least on one other
> target.  Note the cases you remove the parens aren't just || and &&, but
> most likely also | and & (at least there is some flag whether to print those
> as && or &).  And there is code for the caching of the attributes where the
> result is still usable, I believe the patch doesn't break that, but it
> wouldn't hurt to verify that.

Let's just defer it IMO. What do we care if other compilers are 
terminally broken? Let's use it as marketing material :)


Bernd

Re: [PING] genattrab.c generate switch

[Jakub Jelinek]

On Fri, Mar 04, 2016 at 07:01:10PM +0100, Bernd Schmidt wrote:
> On 03/04/2016 06:56 PM, Jakub Jelinek wrote:
> >I think we don't need to guarantee identical assembly, the reason I've
> >suggested that was if it passed, it would be much easier to verify.
> >Without that, I think it should be bootstrapped at least on one other
> >target.  Note the cases you remove the parens aren't just || and &&, but
> >most likely also | and & (at least there is some flag whether to print those
> >as && or &).  And there is code for the caching of the attributes where the
> >result is still usable, I believe the patch doesn't break that, but it
> >wouldn't hurt to verify that.
> 
> Let's just defer it IMO. What do we care if other compilers are terminally
> broken? Let's use it as marketing material :)

Ok.

	Jakub

Re: [PING] genattrab.c generate switch

[Patrick Palka]

On Fri, Mar 4, 2016 at 12:49 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 03/04/2016 06:27 PM, Bernd Schmidt wrote:
>>
>> On 03/04/2016 06:14 PM, Patrick Palka wrote:
>>
>>> I just quickly tested building the generated insn-attrtab.c with and
>>> without the patch using my host gcc 5.3 compiler and the .s output is
>>> not the same.
>>
>>
>> Hmm, looking at the 003t.original dump it looks like there are
>> differences in SAVE_EXPRs. Indeed we seem to generate different code for
>>
>> int at;
>>
>> int foo ()
>> {
>>    if (at == 2 || at == 4 || at == 7)
>>      return 1;
>>    return 0;
>> }
>>
>> int bar ()
>> {
>>    if (at == 2 || (at == 4 || at == 7))
>>      return 1;
>>    return 0;
>> }
>
>
> Ahh... it's not just different placement of SAVE_EXPRs, it's actually a case
> of TRUTH_ORIF_EXPR vs. TRUTH_OR_EXPR (the distinction is invisible in the
> dumps), the latter being created by fold_range_test. That's a bit of a
> broken optimization what with its inability to see more than two comparisons
> at a time... we convert one ORIF per function, but a different one.
>
>
> Bernd

I've filed PR c/70087, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70087

Re: [PING] genattrab.c generate switch

[Jeff Law]

On 03/04/2016 08:13 AM, Bernd Schmidt wrote:
> On 03/04/2016 03:27 PM, Patrick Palka wrote:
>>>> I still suggest to try making write_test_expr() avoid emitting
>>>> redundant parentheses for chains of || or &&, which would fix the
>>>> original issue all the same.  Previously you claimed that such a
>>>> change would not be simpler than your current patch, but I gave it a
>>>> quick try and ended up with a much smaller patch:
>
> This looks like a reasonable stopgap if a release manager thinks this is
> important enough to fix for gcc-6. Some comments below for that case.
> Longer term I'm not sure - in theory maybe the switch would allow us to
> generate better code, but I tried it and code size actually seems to go
> up (could be jump tables however). I also noticed that in the version
> with the switch we still have cases of
>
> switch (cached_type)
> {
>   ....
>   default:
>     switch (cached_type)
>     {
>     }
> }
>
> so that might be a point where the patch could be improved to see if we
> can get better code generation by collapsing these switches. Might also
> be worth trying to optimize this pattern in gcc.
There's probably a good number of things we could do for this and 
related scenarios.  Essentially DOM and threading are unlikely to do 
anything with this because the path from the outer case into the inner 
switch is going to have multiple values associated with cached_type.

The backwards threader, if it was presented with SSI rather than SSA 
could probably untangle it, but I'm not sure we're ready to make the 
jump to SSI (though it would simplify VRP & DOM).

Jeff

[PATCH] Reduce nesting of parentheses in conditionals generated by genattrtab

[Patrick Palka]

On Fri, Mar 4, 2016 at 12:56 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Mar 04, 2016 at 06:49:58PM +0100, Bernd Schmidt wrote:
>> On 03/04/2016 06:27 PM, Bernd Schmidt wrote:
>> >On 03/04/2016 06:14 PM, Patrick Palka wrote:
>> >
>> >>I just quickly tested building the generated insn-attrtab.c with and
>> >>without the patch using my host gcc 5.3 compiler and the .s output is
>> >>not the same.
>> >
>> >Hmm, looking at the 003t.original dump it looks like there are
>> >differences in SAVE_EXPRs. Indeed we seem to generate different code for
>> >
>> >int at;
>> >
>> >int foo ()
>> >{
>> >   if (at == 2 || at == 4 || at == 7)
>> >     return 1;
>> >   return 0;
>> >}
>> >
>> >int bar ()
>> >{
>> >   if (at == 2 || (at == 4 || at == 7))
>> >     return 1;
>> >   return 0;
>> >}
>>
>> Ahh... it's not just different placement of SAVE_EXPRs, it's actually a case
>> of TRUTH_ORIF_EXPR vs. TRUTH_OR_EXPR (the distinction is invisible in the
>> dumps), the latter being created by fold_range_test. That's a bit of a
>> broken optimization what with its inability to see more than two comparisons
>> at a time... we convert one ORIF per function, but a different one.
>
> I think we don't need to guarantee identical assembly, the reason I've
> suggested that was if it passed, it would be much easier to verify.
> Without that, I think it should be bootstrapped at least on one other
> target.  Note the cases you remove the parens aren't just || and &&, but
> most likely also | and & (at least there is some flag whether to print those
> as && or &).  And there is code for the caching of the attributes where the
> result is still usable, I believe the patch doesn't break that, but it
> wouldn't hurt to verify that.

Here's an updated patch that mentions that & and | are also affected.  And I
can't see how this change would possibly affect the attr caching stuff since it
just makes

  (a) OP ((b) OP (c))

get emitted as

  (a) OP (b) OP (c)

For OP = || or &&, the expressions a, b and c will still get evaluated left to
right.  And for OP = | or &, the order of evaluation of a, b and c remains
undefined.

Is this OK to commit after testing on x86_64-pc-linux-gnu?

gcc/ChangeLog:

	* genattrtab.c (write_test_expr): New parameter EMIT_PARENS
	which defaults to true.  Emit an outer pair of parentheses only if
	EMIT_PARENS.  When continuing a chain of && or || (or & or |),
	don't emit parentheses for the right-hand operand.
---
 gcc/genattrtab.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index b64d8b9..c956527 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -3416,7 +3416,10 @@ find_attrs_to_cache (rtx exp, bool create)
 
 /* Given a piece of RTX, print a C expression to test its truth value to OUTF.
    We use AND and IOR both for logical and bit-wise operations, so
-   interpret them as logical unless they are inside a comparison expression.  */
+   interpret them as logical unless they are inside a comparison expression.
+
+   An outermost pair of parentheses is emitted around this C expression unless
+   EMIT_PARENS is false.  */
 
 /* Interpret AND/IOR as bit-wise operations instead of logical.  */
 #define FLG_BITWISE		1
@@ -3432,16 +3435,16 @@ find_attrs_to_cache (rtx exp, bool create)
 #define FLG_OUTSIDE_AND		8
 
 static unsigned int
-write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags)
+write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags,
+		 bool emit_parens = true)
 {
   int comparison_operator = 0;
   RTX_CODE code;
   struct attr_desc *attr;
 
-  /* In order not to worry about operator precedence, surround our part of
-     the expression with parentheses.  */
+  if (emit_parens)
+    fprintf (outf, "(");
 
-  fprintf (outf, "(");
   code = GET_CODE (exp);
   switch (code)
     {
@@ -3575,8 +3578,18 @@ write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags)
 	      || GET_CODE (XEXP (exp, 1)) == EQ_ATTR
 	      || (GET_CODE (XEXP (exp, 1)) == NOT
 		  && GET_CODE (XEXP (XEXP (exp, 1), 0)) == EQ_ATTR)))
-	attrs_cached
-	  = write_test_expr (outf, XEXP (exp, 1), attrs_cached, flags);
+	{
+	  bool need_parens = true;
+
+	  /* No need to emit parentheses around the right-hand operand if we are
+	     continuing a chain of && or || (or & or |).  */
+	  if (GET_CODE (XEXP (exp, 1)) == code)
+	    need_parens = false;
+
+	  attrs_cached
+	    = write_test_expr (outf, XEXP (exp, 1), attrs_cached, flags,
+			       need_parens);
+	}
       else
 	write_test_expr (outf, XEXP (exp, 1), attrs_cached,
 			 flags | comparison_operator);
@@ -3794,7 +3807,9 @@ write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags)
 	     GET_RTX_NAME (code));
     }
 
-  fprintf (outf, ")");
+  if (emit_parens)
+    fprintf (outf, ")");
+
   return attrs_cached;
 }
 
-- 
2.8.1.361.g2fbef4c

Re: [PATCH] Reduce nesting of parentheses in conditionals generated by genattrtab

[Jakub Jelinek]

On Wed, Apr 27, 2016 at 09:23:47AM -0400, Patrick Palka wrote:
> Here's an updated patch that mentions that & and | are also affected.  And I
> can't see how this change would possibly affect the attr caching stuff since it
> just makes
> 
>   (a) OP ((b) OP (c))
> 
> get emitted as
> 
>   (a) OP (b) OP (c)
> 
> For OP = || or &&, the expressions a, b and c will still get evaluated left to
> right.  And for OP = | or &, the order of evaluation of a, b and c remains
> undefined.
> 
> Is this OK to commit after testing on x86_64-pc-linux-gnu?
> 
> gcc/ChangeLog:
> 
> 	* genattrtab.c (write_test_expr): New parameter EMIT_PARENS
> 	which defaults to true.  Emit an outer pair of parentheses only if
> 	EMIT_PARENS.  When continuing a chain of && or || (or & or |),
> 	don't emit parentheses for the right-hand operand.

Ok for trunk.

	Jakub