Re: [PATCH] Fix 416.gamess miscompare (PR71311)
On June 2, 2016 9:05:36 PM GMT+02:00, Richard Biener wrote: >On June 2, 2016 7:47:19 PM GMT+02:00, Bernhard Reutner-Fischer > wrote: >>On June 1, 2016 9:37:26 AM GMT+02:00, Richard Biener >> wrote: >> PR tree-optimization/71311 * genmatch.c (comparison_code_p): New predicate. >> >>TREE_CODE_CLASS (code) == tcc_comparison > >Only if I'd pull all of the tree stuff into the generator. Ah, I thought this already was in there.
Re: [PATCH] Fix 416.gamess miscompare (PR71311)
On June 2, 2016 7:47:19 PM GMT+02:00, Bernhard Reutner-Fischer wrote: >On June 1, 2016 9:37:26 AM GMT+02:00, Richard Biener > wrote: > >>> PR tree-optimization/71311 >>> * genmatch.c (comparison_code_p): New predicate. > >TREE_CODE_CLASS (code) == tcc_comparison Only if I'd pull all of the tree stuff into the generator. Richard. >? >thanks,
Re: [PATCH] Fix 416.gamess miscompare (PR71311)
On June 1, 2016 9:37:26 AM GMT+02:00, Richard Biener wrote: >> PR tree-optimization/71311 >> * genmatch.c (comparison_code_p): New predicate. TREE_CODE_CLASS (code) == tcc_comparison ? thanks,
Re: [PATCH] Fix 416.gamess miscompare (PR71311)
On Tue, 31 May 2016, Richard Biener wrote:
> On Tue, 31 May 2016, Richard Biener wrote:
>
> >
> > The following patch adds missing patterns with swapped comparison
> > operands for the @0 < @1 and @0 < @2 to @0 < min (@1, @2) transform.
> > This nullifies the difference gimplify-into-SSA made on
> > rhfuhf.F:ROFOCK which causes the function no longer to be miscompiled
> > (by vectorization eventually, -fno-tree-vectorize also fixes the
> > miscompare at r235817).
>
> The following is a variant, avoiding the pattern repetition in match.pd
> by (finally) completing a local patch I had to support "commutating"
> relational operators. It also adds sanity-checking for what you apply
> :c to which catches a few cases in match.pd I introduce :C for in this
> patch.
>
> Bootstrap / regtest in progress on x86_64-unknown-linux-gnu.
I'm re-testing with enforcing a non-INTEGER_CST @0 on the pattern, thus
/* Transform (@0 < @1 and @0 < @2) to use min,
(@0 > @1 and @0 > @2) to use max */
(for op (lt le gt ge)
ext (min min max max)
(simplify
(bit_and (op:cs @0 @1) (op:cs @0 @2))
(if (INTEGRAL_TYPE_P (TREE_TYPE (@0))
&& TREE_CODE (@0) != INTEGER_CST)
(op @0 (ext @1 @2)
as otherwise we have testsuite regressions and ICEs because
operand_equal_p doesn't enforce type compatibility between INTEGER_CSTs.
Using types_match (@1, @2) still leads to tests like
ssa-ifcombine-ccmp-1.c FAILing in their dump scanning. It's also hardly
profitable to replace a < 0 && b < 0 with max(a,b) < 0 given tests
against constants are going to be way cheaper than computing max.
Meanwhile I've split the relational :c support and committed that,
also testing a followup to replace existing duplications with :c
(will post after testing completed).
Richard.
> Richard.
>
> 2016-05-31 Richard Biener
>
> PR tree-optimization/71311
> * genmatch.c (comparison_code_p): New predicate.
> (swap_tree_comparison): New function.
> (commutate): Add for_vec parameter to append new for entries.
> Support commutating relational operators by swapping it alongside
> operands.
> (lower_commutative): Adjust.
> (dt_simplify::gen): Do not pass artificial operators to gen
> functions.
> (decision_tree::gen): Do not add artificial operators as parameters.
> (parser::parse_expr): Verify operator commutativity when :c is
> applied. Allow :C to override this.
> * match.pd: Adjust patterns to use :C instead of :c where required.
> Add :c to @0 < @1 && @0 < @2 -> @0 < min(@1,@2) transform.
>
> Index: gcc/genmatch.c
> ===
> *** gcc/genmatch.c(revision 236877)
> --- gcc/genmatch.c(working copy)
> *** commutative_ternary_tree_code (enum tree
> *** 291,296
> --- 291,325
> return false;
> }
>
> + /* Return true if CODE is a comparison. */
> +
> + bool
> + comparison_code_p (enum tree_code code)
> + {
> + switch (code)
> + {
> + case EQ_EXPR:
> + case NE_EXPR:
> + case ORDERED_EXPR:
> + case UNORDERED_EXPR:
> + case LTGT_EXPR:
> + case UNEQ_EXPR:
> + case GT_EXPR:
> + case GE_EXPR:
> + case LT_EXPR:
> + case LE_EXPR:
> + case UNGT_EXPR:
> + case UNGE_EXPR:
> + case UNLT_EXPR:
> + case UNLE_EXPR:
> + return true;
> +
> + default:
> + break;
> + }
> + return false;
> + }
> +
>
> /* Base class for all identifiers the parser knows. */
>
> *** get_operator (const char *id, bool allow
> *** 528,533
> --- 557,598
> return operators->find_with_hash (&tem, tem.hashval);
> }
>
> + /* Return the comparison operators that results if the operands are
> +swapped. This is safe for floating-point. */
> +
> + id_base *
> + swap_tree_comparison (operator_id *p)
> + {
> + switch (p->code)
> + {
> + case EQ_EXPR:
> + case NE_EXPR:
> + case ORDERED_EXPR:
> + case UNORDERED_EXPR:
> + case LTGT_EXPR:
> + case UNEQ_EXPR:
> + return p;
> + case GT_EXPR:
> + return get_operator ("LT_EXPR");
> + case GE_EXPR:
> + return get_operator ("LE_EXPR");
> + case LT_EXPR:
> + return get_operator ("GT_EXPR");
> + case LE_EXPR:
> + return get_operator ("GE_EXPR");
> + case UNGT_EXPR:
> + return get_operator ("UNLT_EXPR");
> + case UNGE_EXPR:
> + return get_operator ("UNLE_EXPR");
> + case UNLT_EXPR:
> + return get_operator ("UNGT_EXPR");
> + case UNLE_EXPR:
> + return get_operator ("UNGE_EXPR");
> + default:
> + gcc_unreachable ();
> + }
> + }
> +
> typedef hash_map cid_map_t;
>
>
> *** cartesian_product (const vec< vec *** 816,822
> /* Lower OP to two operands in case it is marked as commutative. */
>
> static vec
> ! commutate (operand *op)
> {
> vec ret = vNULL;
>
> --- 881,887
> /
Re: [PATCH] Fix 416.gamess miscompare (PR71311)
On Tue, 31 May 2016, Richard Biener wrote:
>
> The following patch adds missing patterns with swapped comparison
> operands for the @0 < @1 and @0 < @2 to @0 < min (@1, @2) transform.
> This nullifies the difference gimplify-into-SSA made on
> rhfuhf.F:ROFOCK which causes the function no longer to be miscompiled
> (by vectorization eventually, -fno-tree-vectorize also fixes the
> miscompare at r235817).
The following is a variant, avoiding the pattern repetition in match.pd
by (finally) completing a local patch I had to support "commutating"
relational operators. It also adds sanity-checking for what you apply
:c to which catches a few cases in match.pd I introduce :C for in this
patch.
Bootstrap / regtest in progress on x86_64-unknown-linux-gnu.
Richard.
2016-05-31 Richard Biener
PR tree-optimization/71311
* genmatch.c (comparison_code_p): New predicate.
(swap_tree_comparison): New function.
(commutate): Add for_vec parameter to append new for entries.
Support commutating relational operators by swapping it alongside
operands.
(lower_commutative): Adjust.
(dt_simplify::gen): Do not pass artificial operators to gen
functions.
(decision_tree::gen): Do not add artificial operators as parameters.
(parser::parse_expr): Verify operator commutativity when :c is
applied. Allow :C to override this.
* match.pd: Adjust patterns to use :C instead of :c where required.
Add :c to @0 < @1 && @0 < @2 -> @0 < min(@1,@2) transform.
Index: gcc/genmatch.c
===
*** gcc/genmatch.c (revision 236877)
--- gcc/genmatch.c (working copy)
*** commutative_ternary_tree_code (enum tree
*** 291,296
--- 291,325
return false;
}
+ /* Return true if CODE is a comparison. */
+
+ bool
+ comparison_code_p (enum tree_code code)
+ {
+ switch (code)
+ {
+ case EQ_EXPR:
+ case NE_EXPR:
+ case ORDERED_EXPR:
+ case UNORDERED_EXPR:
+ case LTGT_EXPR:
+ case UNEQ_EXPR:
+ case GT_EXPR:
+ case GE_EXPR:
+ case LT_EXPR:
+ case LE_EXPR:
+ case UNGT_EXPR:
+ case UNGE_EXPR:
+ case UNLT_EXPR:
+ case UNLE_EXPR:
+ return true;
+
+ default:
+ break;
+ }
+ return false;
+ }
+
/* Base class for all identifiers the parser knows. */
*** get_operator (const char *id, bool allow
*** 528,533
--- 557,598
return operators->find_with_hash (&tem, tem.hashval);
}
+ /* Return the comparison operators that results if the operands are
+swapped. This is safe for floating-point. */
+
+ id_base *
+ swap_tree_comparison (operator_id *p)
+ {
+ switch (p->code)
+ {
+ case EQ_EXPR:
+ case NE_EXPR:
+ case ORDERED_EXPR:
+ case UNORDERED_EXPR:
+ case LTGT_EXPR:
+ case UNEQ_EXPR:
+ return p;
+ case GT_EXPR:
+ return get_operator ("LT_EXPR");
+ case GE_EXPR:
+ return get_operator ("LE_EXPR");
+ case LT_EXPR:
+ return get_operator ("GT_EXPR");
+ case LE_EXPR:
+ return get_operator ("GE_EXPR");
+ case UNGT_EXPR:
+ return get_operator ("UNLT_EXPR");
+ case UNGE_EXPR:
+ return get_operator ("UNLE_EXPR");
+ case UNLT_EXPR:
+ return get_operator ("UNGT_EXPR");
+ case UNLE_EXPR:
+ return get_operator ("UNGE_EXPR");
+ default:
+ gcc_unreachable ();
+ }
+ }
+
typedef hash_map cid_map_t;
*** cartesian_product (const vec< vec
! commutate (operand *op)
{
vec ret = vNULL;
--- 881,887
/* Lower OP to two operands in case it is marked as commutative. */
static vec
! commutate (operand *op, vec > &for_vec)
{
vec ret = vNULL;
*** commutate (operand *op)
*** 827,833
ret.safe_push (op);
return ret;
}
! vec v = commutate (c->what);
for (unsigned i = 0; i < v.length (); ++i)
{
capture *nc = new capture (c->location, c->where, v[i]);
--- 892,898
ret.safe_push (op);
return ret;
}
! vec v = commutate (c->what, for_vec);
for (unsigned i = 0; i < v.length (); ++i)
{
capture *nc = new capture (c->location, c->where, v[i]);
*** commutate (operand *op)
*** 845,851
vec< vec > ops_vector = vNULL;
for (unsigned i = 0; i < e->ops.length (); ++i)
! ops_vector.safe_push (commutate (e->ops[i]));
auto_vec< vec > result;
auto_vec v (e->ops.length ());
--- 910,916
vec< vec > ops_vector = vNULL;
for (unsigned i = 0; i < e->ops.length (); ++i)
! ops_vector.safe_push (commutate (e->ops[i], for_vec));
auto_vec< vec > result;
auto_vec v (e->ops.length ());
*** commutate (operand *op)
*** 868,873
--- 933,982
for (unsigned i = 0; i < result.length (); ++i)
{
[PATCH] Fix 416.gamess miscompare (PR71311)
The following patch adds missing patterns with swapped comparison operands for the @0 < @1 and @0 < @2 to @0 < min (@1, @2) transform. This nullifies the difference gimplify-into-SSA made on rhfuhf.F:ROFOCK which causes the function no longer to be miscompiled (by vectorization eventually, -fno-tree-vectorize also fixes the miscompare at r235817). Bootstrap and regtest running on x86_64-unknown-linux-gnu. I didn't attempt to understand the miscompile or create an executable testcase. Richard. 2016-05-31 Richard Biener PR tree-optimization/71311 * match.pd (@0 < @1 and @0 < @2 to @0 < min (@1, @2)): Add cases with swapped comparison operands. Index: gcc/match.pd === --- gcc/match.pd(revision 235817) +++ gcc/match.pd(working copy) @@ -3179,6 +3179,23 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (bit_and (op:s @0 @1) (op:s @0 @2)) (if (INTEGRAL_TYPE_P (TREE_TYPE (@0))) (op @0 (ext @1 @2) +/* Transform (@1 < @0 and @2 < @0) to use max, + (@1 > @0 and @2 > @0) to use min */ +(for op (lt le gt ge) + ext (max max min min) + (simplify + (bit_and (op:s @1 @0) (op:s @2 @0)) + (if (INTEGRAL_TYPE_P (TREE_TYPE (@0))) + (op (ext @1 @2) @0 +/* Transform (@0 < @1 and @2 > @0) to use min, + (@0 > @1 and @2 < @0) to use max */ +(for op (lt le gt ge) + ops (gt ge lt le) + ext (min min max max) + (simplify + (bit_and:c (op:s @0 @1) (ops:s @2 @0)) + (if (INTEGRAL_TYPE_P (TREE_TYPE (@0))) + (op @0 (ext @1 @2) (simplify /* signbit(x) -> 0 if x is nonnegative. */
