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 <rguent...@suse.de>
>
> 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<nofree_string_hash, unsigned> cid_map_t;
>
>
> *************** cartesian_product (const vec< vec<operan
> *** 816,822 ****
> /* Lower OP to two operands in case it is marked as commutative. */
>
> static vec<operand *>
> ! commutate (operand *op)
> {
> vec<operand *> ret = vNULL;
>
> --- 881,887 ----
> /* Lower OP to two operands in case it is marked as commutative. */
>
> static vec<operand *>
> ! commutate (operand *op, vec<vec<user_id *> > &for_vec)
> {
> vec<operand *> ret = vNULL;
>
> *************** commutate (operand *op)
> *** 827,833 ****
> ret.safe_push (op);
> return ret;
> }
> ! vec<operand *> 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<operand *> 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<operand *> > ops_vector = vNULL;
> for (unsigned i = 0; i < e->ops.length (); ++i)
> ! ops_vector.safe_push (commutate (e->ops[i]));
>
> auto_vec< vec<operand *> > result;
> auto_vec<operand *> v (e->ops.length ());
> --- 910,916 ----
>
> vec< vec<operand *> > 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<operand *> > result;
> auto_vec<operand *> v (e->ops.length ());
> *************** commutate (operand *op)
> *** 868,873 ****
> --- 933,982 ----
> for (unsigned i = 0; i < result.length (); ++i)
> {
> expr *ne = new expr (e);
> + if (operator_id *p = dyn_cast <operator_id *> (ne->operation))
> + {
> + if (comparison_code_p (p->code))
> + ne->operation = swap_tree_comparison (p);
> + }
> + else if (user_id *p = dyn_cast <user_id *> (ne->operation))
> + {
> + bool found_compare = false;
> + for (unsigned j = 0; j < p->substitutes.length (); ++j)
> + if (operator_id *q = dyn_cast <operator_id *> (p->substitutes[j]))
> + {
> + if (comparison_code_p (q->code)
> + && swap_tree_comparison (q) != q)
> + {
> + found_compare = true;
> + break;
> + }
> + }
> + if (found_compare)
> + {
> + user_id *newop = new user_id ("<internal>");
> + for (unsigned j = 0; j < p->substitutes.length (); ++j)
> + {
> + id_base *subst = p->substitutes[j];
> + if (operator_id *q = dyn_cast <operator_id *> (subst))
> + {
> + if (comparison_code_p (q->code))
> + subst = swap_tree_comparison (q);
> + }
> + newop->substitutes.safe_push (subst);
> + }
> + ne->operation = newop;
> + /* Search for 'p' inside the for vector and push 'newop'
> + to the same level. */
> + for (unsigned j = 0; newop && j < for_vec.length (); ++j)
> + for (unsigned k = 0; k < for_vec[j].length (); ++k)
> + if (for_vec[j][k] == p)
> + {
> + for_vec[j].safe_push (newop);
> + newop = NULL;
> + break;
> + }
> + }
> + }
> ne->is_commutative = false;
> // result[i].length () is 2 since e->operation is binary
> for (unsigned j = result[i].length (); j; --j)
> *************** commutate (operand *op)
> *** 884,890 ****
> static void
> lower_commutative (simplify *s, vec<simplify *>& simplifiers)
> {
> ! vec<operand *> matchers = commutate (s->match);
> for (unsigned i = 0; i < matchers.length (); ++i)
> {
> simplify *ns = new simplify (s->kind, matchers[i], s->result,
> --- 993,999 ----
> static void
> lower_commutative (simplify *s, vec<simplify *>& simplifiers)
> {
> ! vec<operand *> matchers = commutate (s->match, s->for_vec);
> for (unsigned i = 0; i < matchers.length (); ++i)
> {
> simplify *ns = new simplify (s->kind, matchers[i], s->result,
> *************** dt_simplify::gen (FILE *f, int indent, b
> *** 3248,3254 ****
> fprintf_indent (f, indent, "if (%s (res_code, res_ops, seq, "
> "valueize, type, captures", info->fname);
> for (unsigned i = 0; i < s->for_subst_vec.length (); ++i)
> ! fprintf (f, ", %s", s->for_subst_vec[i].second->id);
> fprintf (f, "))\n");
> fprintf_indent (f, indent, " return true;\n");
> }
> --- 3357,3364 ----
> fprintf_indent (f, indent, "if (%s (res_code, res_ops, seq, "
> "valueize, type, captures", info->fname);
> for (unsigned i = 0; i < s->for_subst_vec.length (); ++i)
> ! if (s->for_subst_vec[i].first->used)
> ! fprintf (f, ", %s", s->for_subst_vec[i].second->id);
> fprintf (f, "))\n");
> fprintf_indent (f, indent, " return true;\n");
> }
> *************** dt_simplify::gen (FILE *f, int indent, b
> *** 3260,3266 ****
> fprintf (f, ", op%d", i);
> fprintf (f, ", captures");
> for (unsigned i = 0; i < s->for_subst_vec.length (); ++i)
> ! fprintf (f, ", %s", s->for_subst_vec[i].second->id);
> fprintf (f, ");\n");
> fprintf_indent (f, indent, "if (res) return res;\n");
> }
> --- 3370,3379 ----
> fprintf (f, ", op%d", i);
> fprintf (f, ", captures");
> for (unsigned i = 0; i < s->for_subst_vec.length (); ++i)
> ! {
> ! if (s->for_subst_vec[i].first->used)
> ! fprintf (f, ", %s", s->for_subst_vec[i].second->id);
> ! }
> fprintf (f, ");\n");
> fprintf_indent (f, indent, "if (res) return res;\n");
> }
> *************** dt_simplify::gen (FILE *f, int indent, b
> *** 3269,3274 ****
> --- 3382,3389 ----
> {
> for (unsigned i = 0; i < s->for_subst_vec.length (); ++i)
> {
> + if (! s->for_subst_vec[i].first->used)
> + continue;
> if (is_a <operator_id *> (s->for_subst_vec[i].second))
> fprintf_indent (f, indent, "enum tree_code %s = %s;\n",
> s->for_subst_vec[i].first->id,
> *************** decision_tree::gen (FILE *f, bool gimple
> *** 3425,3430 ****
> --- 3540,3547 ----
> }
> for (unsigned i = 0; i < s->s->s->for_subst_vec.length (); ++i)
> {
> + if (! s->s->s->for_subst_vec[i].first->used)
> + continue;
> if (is_a <operator_id *> (s->s->s->for_subst_vec[i].second))
> fprintf (f, ", enum tree_code ARG_UNUSED (%s)",
> s->s->s->for_subst_vec[i].first->id);
> *************** parser::parse_expr ()
> *** 3885,3890 ****
> --- 4002,4031 ----
> while (*sp)
> {
> if (*sp == 'c')
> + {
> + if (operator_id *p
> + = dyn_cast<operator_id *> (e->operation))
> + {
> + if (!commutative_tree_code (p->code)
> + && !comparison_code_p (p->code))
> + fatal_at (token, "operation is not commutative");
> + }
> + else if (user_id *p = dyn_cast<user_id *> (e->operation))
> + for (unsigned i = 0;
> + i < p->substitutes.length (); ++i)
> + {
> + if (operator_id *q
> + = dyn_cast<operator_id *> (p->substitutes[i]))
> + {
> + if (!commutative_tree_code (q->code)
> + && !comparison_code_p (q->code))
> + fatal_at (token, "operation %s is not "
> + "commutative", q->id);
> + }
> + }
> + is_commutative = true;
> + }
> + else if (*sp == 'C')
> is_commutative = true;
> else if (*sp == 's')
> {
> Index: gcc/match.pd
> ===================================================================
> *** gcc/match.pd (revision 236877)
> --- gcc/match.pd (working copy)
> *************** DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> *** 189,195 ****
> /* Optimize -A / A to -1.0 if we don't care about
> NaNs or Infinities. */
> (simplify
> ! (rdiv:c @0 (negate @0))
> (if (FLOAT_TYPE_P (type)
> && ! HONOR_NANS (type)
> && ! HONOR_INFINITIES (type))
> --- 189,195 ----
> /* Optimize -A / A to -1.0 if we don't care about
> NaNs or Infinities. */
> (simplify
> ! (rdiv:C @0 (negate @0))
> (if (FLOAT_TYPE_P (type)
> && ! HONOR_NANS (type)
> && ! HONOR_INFINITIES (type))
> *************** DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> *** 2811,2817 ****
>
> /* cabs(x+0i) or cabs(0+xi) -> abs(x). */
> (simplify
> ! (CABS (complex:c @0 real_zerop@1))
> (abs @0))
>
> /* trunc(trunc(x)) -> trunc(x), etc. */
> --- 2833,2839 ----
>
> /* cabs(x+0i) or cabs(0+xi) -> abs(x). */
> (simplify
> ! (CABS (complex:C @0 real_zerop@1))
> (abs @0))
>
> /* trunc(trunc(x)) -> trunc(x), etc. */
> *************** DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> *** 3246,3252 ****
> (for op (lt le gt ge)
> ext (min min max max)
> (simplify
> ! (bit_and (op:s @0 @1) (op:s @0 @2))
> (if (INTEGRAL_TYPE_P (TREE_TYPE (@0)))
> (op @0 (ext @1 @2)))))
>
> --- 3268,3274 ----
> (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)))
> (op @0 (ext @1 @2)))))
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
