diff mbox

[PING] genattrab.c generate switch

Message ID alpine.DEB.2.20.9.1603041122341.1231@idea
State New
Headers show

Commit Message

Patrick Palka March 4, 2016, 4:34 p.m. UTC 
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(-)

Comments

Jakub Jelinek March 4, 2016, 4:42 p.m. UTC | #1 
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
Patrick Palka March 4, 2016, 5:14 p.m. UTC | #2 
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.
Bernd Schmidt March 4, 2016, 5:27 p.m. UTC | #3 
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
Bernd Schmidt March 4, 2016, 5:49 p.m. UTC | #4 
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
Jakub Jelinek March 4, 2016, 5:56 p.m. UTC | #5 
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
Bernd Schmidt March 4, 2016, 6:01 p.m. UTC | #6 
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
Jakub Jelinek March 4, 2016, 6:02 p.m. UTC | #7 
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
Patrick Palka March 4, 2016, 8:16 p.m. UTC | #8 
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
diff mbox

Patch

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;
 }