On Fri, Mar 4, 2016 at 12:56 PM, Jakub Jelinek <ja...@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
