| Message ID | 20191204085559.GL10088@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Some further match.pd optimizations with nop_convert (PR tree-optimization/92734) | expand |
On Wed, 4 Dec 2019, Jakub Jelinek wrote: > Hi! > > The following optimizations also can't handle nop conversions between the > inner and outer addition/subtraction, and even -fwrapv doesn't help there, > only simplify-rtx.c is able to do something about those. > > Implemented thusly, bootstrapped/regtested on x86_64-linux and i686-linux, > ok for trunk? KO. Richard. > 2019-12-04 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/92734 > * match.pd ((A +- B) - A -> +- B, (A +- B) -+ B -> A, > A - (A +- B) -> -+ B, A +- (B -+ A) -> +- B): Handle nop_convert. > > * gcc.dg/tree-ssa/pr92734-2.c: New test. > > --- gcc/match.pd.jj 2019-12-03 10:20:00.244913801 +0100 > +++ gcc/match.pd 2019-12-03 13:36:01.084435697 +0100 > @@ -2159,20 +2159,26 @@ (define_operator_list COND_TERNARY > /* A - (A +- B) -> -+ B */ > /* A +- (B -+ A) -> +- B */ > (simplify > - (minus (plus:c @0 @1) @0) > - @1) > + (minus (nop_convert (plus:c (nop_convert @0) @1)) @0) > + (view_convert @1)) > (simplify > - (minus (minus @0 @1) @0) > - (negate @1)) > + (minus (nop_convert (minus (nop_convert @0) @1)) @0) > + (if (!ANY_INTEGRAL_TYPE_P (type) > + || TYPE_OVERFLOW_WRAPS (type)) > + (negate (view_convert @1)) > + (view_convert (negate @1)))) > (simplify > - (plus:c (minus @0 @1) @1) > - @0) > + (plus:c (nop_convert (minus @0 (nop_convert @1))) @1) > + (view_convert @0)) > (simplify > - (minus @0 (plus:c @0 @1)) > - (negate @1)) > + (minus @0 (nop_convert (plus:c (nop_convert @0) @1))) > + (if (!ANY_INTEGRAL_TYPE_P (type) > + || TYPE_OVERFLOW_WRAPS (type)) > + (negate (view_convert @1)) > + (view_convert (negate @1)))) > (simplify > - (minus @0 (minus @0 @1)) > - @1) > + (minus @0 (nop_convert (minus (nop_convert @0) @1))) > + (view_convert @1)) > /* (A +- B) + (C - A) -> C +- B */ > /* (A + B) - (A - C) -> B + C */ > /* More cases are handled with comparisons. */ > --- gcc/testsuite/gcc.dg/tree-ssa/pr92734-2.c.jj 2019-12-03 13:56:05.036800119 +0100 > +++ gcc/testsuite/gcc.dg/tree-ssa/pr92734-2.c 2019-12-03 13:55:43.035140773 +0100 > @@ -0,0 +1,76 @@ > +/* PR tree-optimization/92734 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-tree-optimized" } */ > +/* Verify there are no binary additions or subtractions left. There can > + be just casts and negations. */ > +/* { dg-final { scan-tree-dump-not " \[+-] " "optimized" } } */ > + > +int > +f1 (int x, unsigned y) > +{ > + int a = x + y; > + return a - x; > +} > + > +unsigned > +f2 (unsigned x, int y) > +{ > + unsigned a = (int) x + y; > + return a - x; > +} > + > +int > +f3 (int x, unsigned y) > +{ > + int a = x - y; > + return a - x; > +} > + > +unsigned > +f4 (unsigned x, int y) > +{ > + unsigned a = (int) x - y; > + return a - x; > +} > + > +int > +f5 (unsigned x, int y) > +{ > + int a = x - y; > + return a + y; > +} > + > +unsigned > +f6 (int x, unsigned y) > +{ > + unsigned a = x - (int) y; > + return a + y; > +} > + > +int > +f7 (int x, unsigned y) > +{ > + int a = x + y; > + return x - a; > +} > + > +unsigned > +f8 (unsigned x, int y) > +{ > + unsigned a = (int) x + y; > + return x - a; > +} > + > +int > +f9 (int x, unsigned y) > +{ > + int a = x - y; > + return x - a; > +} > + > +unsigned > +f10 (unsigned x, int y) > +{ > + unsigned a = (int) x - y; > + return x - a; > +} > > Jakub > >
On Wed, 4 Dec 2019, Jakub Jelinek wrote: > --- gcc/match.pd.jj 2019-12-03 10:20:00.244913801 +0100 > +++ gcc/match.pd 2019-12-03 13:36:01.084435697 +0100 > @@ -2159,20 +2159,26 @@ (define_operator_list COND_TERNARY > /* A - (A +- B) -> -+ B */ > /* A +- (B -+ A) -> +- B */ > (simplify > - (minus (plus:c @0 @1) @0) > - @1) > + (minus (nop_convert (plus:c (nop_convert @0) @1)) @0) > + (view_convert @1)) I know I introduced nop_convert, and it can be convenient, but IIRC it also has some limitations. int f(int b,unsigned c){ int a=c; int d=a+b; return d-a; } int g(int a, int b){ int d=(unsigned)a+b; return d-a; } int h(int b,int a){ int d=a+b; return d-a; } While g and h are properly optimized during forwprop1, f isn't, because nop_convert sees that 'a' comes from a conversion, and only returns d (unlike 'convert?' which would try both a and d). When inside nop_convert we have an operation, say (nop_convert (plus ...)), there is no ambiguity, so we are fine. With just (nop_convert @0), less so. It happens that during EVRP, for some reason (different valuization function?), nop_convert does not match the conversion, and we do optimize f. But that almost looks like an accident. convert? with explicit checks would probably work better for the inner conversion, except that handling the vector view_convert case may become painful. I didn't think too hard about possible fancy tricks like a second nop_convert: (minus (nop_convert (plus:c (nop_convert @0) @1)) (nop_convert @0))
On December 5, 2019 2:03:24 PM GMT+01:00, Marc Glisse <marc.glisse@inria.fr> wrote: >On Wed, 4 Dec 2019, Jakub Jelinek wrote: > >> --- gcc/match.pd.jj 2019-12-03 10:20:00.244913801 +0100 >> +++ gcc/match.pd 2019-12-03 13:36:01.084435697 +0100 >> @@ -2159,20 +2159,26 @@ (define_operator_list COND_TERNARY >> /* A - (A +- B) -> -+ B */ >> /* A +- (B -+ A) -> +- B */ >> (simplify >> - (minus (plus:c @0 @1) @0) >> - @1) >> + (minus (nop_convert (plus:c (nop_convert @0) @1)) @0) >> + (view_convert @1)) > >I know I introduced nop_convert, and it can be convenient, but IIRC it >also has some limitations. > >int f(int b,unsigned c){ > int a=c; > int d=a+b; > return d-a; >} >int g(int a, int b){ > int d=(unsigned)a+b; > return d-a; >} >int h(int b,int a){ > int d=a+b; > return d-a; >} > >While g and h are properly optimized during forwprop1, f isn't, because > >nop_convert sees that 'a' comes from a conversion, and only returns d >(unlike 'convert?' which would try both a and d). > >When inside nop_convert we have an operation, say (nop_convert (plus >...)), there is no ambiguity, so we are fine. With just (nop_convert >@0), >less so. > >It happens that during EVRP, for some reason (different valuization >function?), nop_convert does not match the conversion, and we do >optimize >f. But that almost looks like an accident. > >convert? with explicit checks would probably work better for the inner >conversion, except that handling the vector view_convert case may >become >painful. I didn't think too hard about possible fancy tricks like a >second >nop_convert: > >(minus (nop_convert (plus:c (nop_convert @0) @1)) (nop_convert @0)) If use gets more and more we can make nop_convert a first class citizen and allow a? Variant. Or handle all predicates as? Variant. Richard.
On Thu, Dec 5, 2019 at 4:16 PM Richard Biener <rguenther@suse.de> wrote: > > On December 5, 2019 2:03:24 PM GMT+01:00, Marc Glisse <marc.glisse@inria.fr> wrote: > >On Wed, 4 Dec 2019, Jakub Jelinek wrote: > > > >> --- gcc/match.pd.jj 2019-12-03 10:20:00.244913801 +0100 > >> +++ gcc/match.pd 2019-12-03 13:36:01.084435697 +0100 > >> @@ -2159,20 +2159,26 @@ (define_operator_list COND_TERNARY > >> /* A - (A +- B) -> -+ B */ > >> /* A +- (B -+ A) -> +- B */ > >> (simplify > >> - (minus (plus:c @0 @1) @0) > >> - @1) > >> + (minus (nop_convert (plus:c (nop_convert @0) @1)) @0) > >> + (view_convert @1)) > > > >I know I introduced nop_convert, and it can be convenient, but IIRC it > >also has some limitations. > > > >int f(int b,unsigned c){ > > int a=c; > > int d=a+b; > > return d-a; > >} > >int g(int a, int b){ > > int d=(unsigned)a+b; > > return d-a; > >} > >int h(int b,int a){ > > int d=a+b; > > return d-a; > >} > > > >While g and h are properly optimized during forwprop1, f isn't, because > > > >nop_convert sees that 'a' comes from a conversion, and only returns d > >(unlike 'convert?' which would try both a and d). > > > >When inside nop_convert we have an operation, say (nop_convert (plus > >...)), there is no ambiguity, so we are fine. With just (nop_convert > >@0), > >less so. > > > >It happens that during EVRP, for some reason (different valuization > >function?), nop_convert does not match the conversion, and we do > >optimize > >f. But that almost looks like an accident. > > > >convert? with explicit checks would probably work better for the inner > >conversion, except that handling the vector view_convert case may > >become > >painful. I didn't think too hard about possible fancy tricks like a > >second > >nop_convert: > > > >(minus (nop_convert (plus:c (nop_convert @0) @1)) (nop_convert @0)) > > If use gets more and more we can make nop_convert a first class citizen and allow a? Variant. Or handle all predicates as? Variant. Like the attached (need to adjust docs & rename some functions still) which generalizes [digit]? parsing. This allows you to write (nop_convert? ...) It works (generated files are unchanged), so I guess I'll push it after polishing. It also extends the 1/2/3 grouping to be able to group like (plus (nop_convert2? @0) (convert2? @1)) so either both will be present or none (meaning that when grouping you can choose the "cheaper" test for one in case you know the conversions will be the same). Richard. > Richard. >
On Fri, 6 Dec 2019, Richard Biener wrote: > On Thu, Dec 5, 2019 at 4:16 PM Richard Biener <rguenther@suse.de> wrote: > > > > On December 5, 2019 2:03:24 PM GMT+01:00, Marc Glisse <marc.glisse@inria.fr> wrote: > > >On Wed, 4 Dec 2019, Jakub Jelinek wrote: > > > > > >> --- gcc/match.pd.jj 2019-12-03 10:20:00.244913801 +0100 > > >> +++ gcc/match.pd 2019-12-03 13:36:01.084435697 +0100 > > >> @@ -2159,20 +2159,26 @@ (define_operator_list COND_TERNARY > > >> /* A - (A +- B) -> -+ B */ > > >> /* A +- (B -+ A) -> +- B */ > > >> (simplify > > >> - (minus (plus:c @0 @1) @0) > > >> - @1) > > >> + (minus (nop_convert (plus:c (nop_convert @0) @1)) @0) > > >> + (view_convert @1)) > > > > > >I know I introduced nop_convert, and it can be convenient, but IIRC it > > >also has some limitations. > > > > > >int f(int b,unsigned c){ > > > int a=c; > > > int d=a+b; > > > return d-a; > > >} > > >int g(int a, int b){ > > > int d=(unsigned)a+b; > > > return d-a; > > >} > > >int h(int b,int a){ > > > int d=a+b; > > > return d-a; > > >} > > > > > >While g and h are properly optimized during forwprop1, f isn't, because > > > > > >nop_convert sees that 'a' comes from a conversion, and only returns d > > >(unlike 'convert?' which would try both a and d). > > > > > >When inside nop_convert we have an operation, say (nop_convert (plus > > >...)), there is no ambiguity, so we are fine. With just (nop_convert > > >@0), > > >less so. > > > > > >It happens that during EVRP, for some reason (different valuization > > >function?), nop_convert does not match the conversion, and we do > > >optimize > > >f. But that almost looks like an accident. > > > > > >convert? with explicit checks would probably work better for the inner > > >conversion, except that handling the vector view_convert case may > > >become > > >painful. I didn't think too hard about possible fancy tricks like a > > >second > > >nop_convert: > > > > > >(minus (nop_convert (plus:c (nop_convert @0) @1)) (nop_convert @0)) > > > > If use gets more and more we can make nop_convert a first class citizen and allow a? Variant. Or handle all predicates as? Variant. > > Like the attached (need to adjust docs & rename some functions still) > which generalizes > [digit]? parsing. This allows you to write (nop_convert? ...) > > It works (generated files are unchanged), so I guess I'll push it > after polishing. > > It also extends the 1/2/3 grouping to be able to group like (plus > (nop_convert2? @0) (convert2? @1)) > so either both will be present or none (meaning that when grouping you > can choose the "cheaper" > test for one in case you know the conversions will be the same). Like this. Bootstrap on x86_64-unknown-linux-gnu running, tested by building docs and comparing {gimple,generic}-match.c before/after the patch. Richard. 2019-12-06 Richard Biener <rguenther@suse.de> * genmatch.c (enum tree_code): Remove CONVERT{0,1,2} and VIEW_CONVERT{0,1,2}. (expr::opt_grp): Add and initialize. (lower_opt_convert): Rename to ... (lower_opt): ... and work on opt_grp, simply switching operations from being optional to being present or not. (has_opt_convert): Rename to ... (has_opt): ... and adjust. (parser::parse_operation): Return the optional opt_grp, remove special-casing of conditional operations and more generally parse [digit]'?'. (parser::parse_expr): Stick on the parsed opt_grp and perform rough verification. (parser::parse_for): Remove now unnecessary code. (main): Likewise. * doc/match-and-simplify.texi: Mention ? now works on all unary operations and also match predicates. Index: gcc/genmatch.c =================================================================== --- gcc/genmatch.c (revision 279036) +++ gcc/genmatch.c (working copy) @@ -224,12 +224,6 @@ output_line_directive (FILE *f, location #define DEFTREECODE(SYM, STRING, TYPE, NARGS) SYM, enum tree_code { #include "tree.def" -CONVERT0, -CONVERT1, -CONVERT2, -VIEW_CONVERT0, -VIEW_CONVERT1, -VIEW_CONVERT2, MAX_TREE_CODES }; #undef DEFTREECODE @@ -703,11 +697,12 @@ public: expr (id_base *operation_, location_t loc, bool is_commutative_ = false) : operand (OP_EXPR, loc), operation (operation_), ops (vNULL), expr_type (NULL), is_commutative (is_commutative_), - is_generic (false), force_single_use (false) {} + is_generic (false), force_single_use (false), opt_grp (0) {} expr (expr *e) : operand (OP_EXPR, e->location), operation (e->operation), ops (vNULL), expr_type (e->expr_type), is_commutative (e->is_commutative), - is_generic (e->is_generic), force_single_use (e->force_single_use) {} + is_generic (e->is_generic), force_single_use (e->force_single_use), + opt_grp (e->opt_grp) {} void append_op (operand *op) { ops.safe_push (op); } /* The operator and its operands. */ id_base *operation; @@ -722,6 +717,8 @@ public: /* Whether pushing any stmt to the sequence should be conditional on this expression having a single-use. */ bool force_single_use; + /* If non-zero, the group for optional handling. */ + unsigned char opt_grp; virtual void gen_transform (FILE *f, int, const char *, bool, int, const char *, capture_info *, dt_operand ** = 0, int = 0); @@ -1093,18 +1090,17 @@ lower_commutative (simplify *s, vec<simp } } -/* Strip conditional conversios using operator OPER from O and its - children if STRIP, else replace them with an unconditional convert. */ +/* Strip conditional operations using group GRP from O and its + children if STRIP, else replace them with an unconditional operation. */ operand * -lower_opt_convert (operand *o, enum tree_code oper, - enum tree_code to_oper, bool strip) +lower_opt (operand *o, unsigned char grp, bool strip) { if (capture *c = dyn_cast<capture *> (o)) { if (c->what) return new capture (c->location, c->where, - lower_opt_convert (c->what, oper, to_oper, strip), + lower_opt (c->what, grp, strip), c->value_match); else return c; @@ -1114,36 +1110,34 @@ lower_opt_convert (operand *o, enum tree if (!e) return o; - if (*e->operation == oper) + if (e->opt_grp == grp) { if (strip) - return lower_opt_convert (e->ops[0], oper, to_oper, strip); + return lower_opt (e->ops[0], grp, strip); expr *ne = new expr (e); - ne->operation = (to_oper == CONVERT_EXPR - ? get_operator ("CONVERT_EXPR") - : get_operator ("VIEW_CONVERT_EXPR")); - ne->append_op (lower_opt_convert (e->ops[0], oper, to_oper, strip)); + ne->opt_grp = 0; + ne->append_op (lower_opt (e->ops[0], grp, strip)); return ne; } expr *ne = new expr (e); for (unsigned i = 0; i < e->ops.length (); ++i) - ne->append_op (lower_opt_convert (e->ops[i], oper, to_oper, strip)); + ne->append_op (lower_opt (e->ops[i], grp, strip)); return ne; } -/* Determine whether O or its children uses the conditional conversion - operator OPER. */ +/* Determine whether O or its children uses the conditional operation + group GRP. */ static bool -has_opt_convert (operand *o, enum tree_code oper) +has_opt (operand *o, unsigned char grp) { if (capture *c = dyn_cast<capture *> (o)) { if (c->what) - return has_opt_convert (c->what, oper); + return has_opt (c->what, grp); else return false; } @@ -1152,11 +1146,11 @@ has_opt_convert (operand *o, enum tree_c if (!e) return false; - if (*e->operation == oper) + if (e->opt_grp == grp) return true; for (unsigned i = 0; i < e->ops.length (); ++i) - if (has_opt_convert (e->ops[i], oper)) + if (has_opt (e->ops[i], grp)) return true; return false; @@ -1166,34 +1160,24 @@ has_opt_convert (operand *o, enum tree_c if required. */ static vec<operand *> -lower_opt_convert (operand *o) +lower_opt (operand *o) { vec<operand *> v1 = vNULL, v2; v1.safe_push (o); - enum tree_code opers[] - = { CONVERT0, CONVERT_EXPR, - CONVERT1, CONVERT_EXPR, - CONVERT2, CONVERT_EXPR, - VIEW_CONVERT0, VIEW_CONVERT_EXPR, - VIEW_CONVERT1, VIEW_CONVERT_EXPR, - VIEW_CONVERT2, VIEW_CONVERT_EXPR }; - - /* Conditional converts are lowered to a pattern with the - conversion and one without. The three different conditional - convert codes are lowered separately. */ + /* Conditional operations are lowered to a pattern with the + operation and one without. All different conditional operation + groups are lowered separately. */ - for (unsigned i = 0; i < sizeof (opers) / sizeof (enum tree_code); i += 2) + for (unsigned i = 1; i <= 10; ++i) { v2 = vNULL; for (unsigned j = 0; j < v1.length (); ++j) - if (has_opt_convert (v1[j], opers[i])) + if (has_opt (v1[j], i)) { - v2.safe_push (lower_opt_convert (v1[j], - opers[i], opers[i+1], false)); - v2.safe_push (lower_opt_convert (v1[j], - opers[i], opers[i+1], true)); + v2.safe_push (lower_opt (v1[j], i, false)); + v2.safe_push (lower_opt (v1[j], i, true)); } if (v2 != vNULL) @@ -1211,9 +1195,9 @@ lower_opt_convert (operand *o) the resulting multiple patterns to SIMPLIFIERS. */ static void -lower_opt_convert (simplify *s, vec<simplify *>& simplifiers) +lower_opt (simplify *s, vec<simplify *>& simplifiers) { - vec<operand *> matchers = lower_opt_convert (s->match); + vec<operand *> matchers = lower_opt (s->match); for (unsigned i = 0; i < matchers.length (); ++i) { simplify *ns = new simplify (s->kind, s->id, matchers[i], s->result, @@ -1557,7 +1541,7 @@ lower (vec<simplify *>& simplifiers, boo { auto_vec<simplify *> out_simplifiers; for (unsigned i = 0; i < simplifiers.length (); ++i) - lower_opt_convert (simplifiers[i], out_simplifiers); + lower_opt (simplifiers[i], out_simplifiers); simplifiers.truncate (0); for (unsigned i = 0; i < out_simplifiers.length (); ++i) @@ -3966,7 +3950,7 @@ private: unsigned get_internal_capture_id (); - id_base *parse_operation (); + id_base *parse_operation (unsigned char &); operand *parse_capture (operand *, bool); operand *parse_expr (); c_expr *parse_c_expr (cpp_ttype); @@ -4157,47 +4141,36 @@ parser::record_operlist (location_t loc, convert2? */ id_base * -parser::parse_operation () +parser::parse_operation (unsigned char &opt_grp) { const cpp_token *id_tok = peek (); + char *alt_id = NULL; const char *id = get_ident (); const cpp_token *token = peek (); - if (strcmp (id, "convert0") == 0) - fatal_at (id_tok, "use 'convert?' here"); - else if (strcmp (id, "view_convert0") == 0) - fatal_at (id_tok, "use 'view_convert?' here"); + opt_grp = 0; if (token->type == CPP_QUERY && !(token->flags & PREV_WHITE)) { - if (strcmp (id, "convert") == 0) - id = "convert0"; - else if (strcmp (id, "convert1") == 0) - ; - else if (strcmp (id, "convert2") == 0) - ; - else if (strcmp (id, "view_convert") == 0) - id = "view_convert0"; - else if (strcmp (id, "view_convert1") == 0) - ; - else if (strcmp (id, "view_convert2") == 0) - ; - else - fatal_at (id_tok, "non-convert operator conditionalized"); - if (!parsing_match_operand) fatal_at (id_tok, "conditional convert can only be used in " "match expression"); + if (ISDIGIT (id[strlen (id) - 1])) + { + opt_grp = id[strlen (id) - 1] - '0' + 1; + alt_id = xstrdup (id); + alt_id[strlen (id) - 1] = '\0'; + if (opt_grp == 1) + fatal_at (id_tok, "use '%s?' here", alt_id); + } + else + opt_grp = 1; eat_token (CPP_QUERY); } - else if (strcmp (id, "convert1") == 0 - || strcmp (id, "convert2") == 0 - || strcmp (id, "view_convert1") == 0 - || strcmp (id, "view_convert2") == 0) - fatal_at (id_tok, "expected '?' after conditional operator"); - id_base *op = get_operator (id); + id_base *op = get_operator (alt_id ? alt_id : id); if (!op) - fatal_at (id_tok, "unknown operator %s", id); - + fatal_at (id_tok, "unknown operator %s", alt_id ? alt_id : id); + if (alt_id) + free (alt_id); user_id *p = dyn_cast<user_id *> (op); if (p && p->is_oper_list) { @@ -4253,7 +4226,8 @@ class operand * parser::parse_expr () { const cpp_token *token = peek (); - expr *e = new expr (parse_operation (), token->src_loc); + unsigned char opt_grp; + expr *e = new expr (parse_operation (opt_grp), token->src_loc); token = peek (); operand *op; bool is_commutative = false; @@ -4349,6 +4323,12 @@ parser::parse_expr () "commutative"); } e->expr_type = expr_type; + if (opt_grp != 0) + { + if (e->ops.length () != 1) + fatal_at (token, "only unary operations can be conditional"); + e->opt_grp = opt_grp; + } return op; } else if (!(token->flags & PREV_WHITE)) @@ -4731,10 +4711,6 @@ parser::parse_for (location_t) id_base *idb = get_operator (oper, true); if (idb == NULL) fatal_at (token, "no such operator '%s'", oper); - if (*idb == CONVERT0 || *idb == CONVERT1 || *idb == CONVERT2 - || *idb == VIEW_CONVERT0 || *idb == VIEW_CONVERT1 - || *idb == VIEW_CONVERT2) - fatal_at (token, "conditional operators cannot be used inside for"); if (arity == -1) arity = idb->nargs; @@ -5141,12 +5117,6 @@ main (int argc, char **argv) add_operator (SYM, # SYM, # TYPE, NARGS); #define END_OF_BASE_TREE_CODES #include "tree.def" -add_operator (CONVERT0, "convert0", "tcc_unary", 1); -add_operator (CONVERT1, "convert1", "tcc_unary", 1); -add_operator (CONVERT2, "convert2", "tcc_unary", 1); -add_operator (VIEW_CONVERT0, "view_convert0", "tcc_unary", 1); -add_operator (VIEW_CONVERT1, "view_convert1", "tcc_unary", 1); -add_operator (VIEW_CONVERT2, "view_convert2", "tcc_unary", 1); #undef END_OF_BASE_TREE_CODES #undef DEFTREECODE Index: gcc/doc/match-and-simplify.texi =================================================================== --- gcc/doc/match-and-simplify.texi (revision 279036) +++ gcc/doc/match-and-simplify.texi (working copy) @@ -380,6 +380,9 @@ patterns only. If you want to match all have access to two additional conditional converts as in @code{(eq (convert1@? @@1) (convert2@? @@2))}. +The support for @code{?} marking extends to all unary operations +including predicates you declare yourself with @code{match}. + Predicates available from the GCC middle-end need to be made available explicitely via @code{define_predicates}:
On Fri, 6 Dec 2019, Richard Biener wrote: >>> nop_convert sees that 'a' comes from a conversion, and only returns d >>> (unlike 'convert?' which would try both a and d). Maybe I should have formulated it as: nop_convert works kind of like a strip_nops. >> If use gets more and more we can make nop_convert a first class citizen and allow a? Variant. One reason I did not specifically push for that is that nop_convert is seldom the right condition. It is convenient because it is usually easy enough to check that it is correct, but in most cases one of narrowing / zero-extension / sign-extension also works. Still, it is better to handle just NOPs than no conversion at all, so I guess making that easy is still good. > Like the attached (need to adjust docs & rename some functions still) > which generalizes > [digit]? parsing. This allows you to write (nop_convert? ...) I guess once this is in, we should replace all (most?) 'nop_convert' with 'nop_convert?' (and possibly a digit in some places) and remove the last alternative in the definition of nop_convert. Although that will increase the code size. In case, we could still have both a nop_convert and a strip_nop predicates. Just thinking: (match (nop_convert @0) (convert @0) (if (tree_nop_conversion_p (type, TREE_TYPE (@0))))) (match (nop_convert @0) (view_convert @0) (if (VECTOR_TYPE_P (type) && VECTOR_TYPE_P (TREE_TYPE (@0)) && known_eq (TYPE_VECTOR_SUBPARTS (type), TYPE_VECTOR_SUBPARTS (TREE_TYPE (@0))) && tree_nop_conversion_p (TREE_TYPE (type), TREE_TYPE (TREE_TYPE (@0)))))) (match (strip_nops @0) (nop_convert? @0)) but that relies on the fact that when there is an optional operation, the machinery first tries with the operation, and then without, the order matters. Maybe being explicit on the priorities is safer (match (strip_nops @0) (nop_convert @0)) (match (strip_nops @0) @0) > It works (generated files are unchanged), so I guess I'll push it > after polishing. > > It also extends the 1/2/3 grouping to be able to group like (plus > (nop_convert2? @0) (convert2? @1)) > so either both will be present or none (meaning that when grouping you > can choose the "cheaper" > test for one in case you know the conversions will be the same). Nice.
On Fri, 6 Dec 2019, Marc Glisse wrote: > On Fri, 6 Dec 2019, Richard Biener wrote: > > >>> nop_convert sees that 'a' comes from a conversion, and only returns d > >>> (unlike 'convert?' which would try both a and d). > > Maybe I should have formulated it as: nop_convert works kind of like a > strip_nops. > > >> If use gets more and more we can make nop_convert a first class citizen and > >> allow a? Variant. > > One reason I did not specifically push for that is that nop_convert is seldom > the right condition. It is convenient because it is usually easy enough to > check that it is correct, but in most cases one of narrowing / zero-extension > / sign-extension also works. Still, it is better to handle just NOPs than no > conversion at all, so I guess making that easy is still good. In my view nop_convert is useful to avoid cluttering the code with (if (tree_nop_conversion_p ...) checks that are even redundant when a (convert? ... is stripped away. > > Like the attached (need to adjust docs & rename some functions still) > > which generalizes > > [digit]? parsing. This allows you to write (nop_convert? ...) > > I guess once this is in, we should replace all (most?) 'nop_convert' with > 'nop_convert?' (and possibly a digit in some places) and remove the last > alternative in the definition of nop_convert. Yes, that was my thinking. > Although that will increase the code size. In case, we could still have both a > nop_convert and a strip_nop predicates. Just thinking: We should measure it, yes, I hope it won't be too bad ;) In theory making genmatch emitted code more like a graph rather than a tree (with shared leafs) might save us a bit here. > (match (nop_convert @0) > (convert @0) > (if (tree_nop_conversion_p (type, TREE_TYPE (@0))))) > (match (nop_convert @0) > (view_convert @0) > (if (VECTOR_TYPE_P (type) && VECTOR_TYPE_P (TREE_TYPE (@0)) > && known_eq (TYPE_VECTOR_SUBPARTS (type), > TYPE_VECTOR_SUBPARTS (TREE_TYPE (@0))) > && tree_nop_conversion_p (TREE_TYPE (type), TREE_TYPE (TREE_TYPE > && (@0)))))) > > (match (strip_nops @0) > (nop_convert? @0)) > > but that relies on the fact that when there is an optional operation, the > machinery first tries with the operation, and then without, the order matters. > Maybe being explicit on the priorities is safer > > (match (strip_nops @0) > (nop_convert @0)) > (match (strip_nops @0) > @0) Yeah, I don't think the above complexity is worth it. > > It works (generated files are unchanged), so I guess I'll push it > > after polishing. > > > > It also extends the 1/2/3 grouping to be able to group like (plus > > (nop_convert2? @0) (convert2? @1)) > > so either both will be present or none (meaning that when grouping you > > can choose the "cheaper" > > test for one in case you know the conversions will be the same). > > Nice. r279037. Richard.
On Fri, 6 Dec 2019, Richard Biener wrote: > On Fri, 6 Dec 2019, Marc Glisse wrote: > > > On Fri, 6 Dec 2019, Richard Biener wrote: > > > > >>> nop_convert sees that 'a' comes from a conversion, and only returns d > > >>> (unlike 'convert?' which would try both a and d). > > > > Maybe I should have formulated it as: nop_convert works kind of like a > > strip_nops. > > > > >> If use gets more and more we can make nop_convert a first class citizen and > > >> allow a? Variant. > > > > One reason I did not specifically push for that is that nop_convert is seldom > > the right condition. It is convenient because it is usually easy enough to > > check that it is correct, but in most cases one of narrowing / zero-extension > > / sign-extension also works. Still, it is better to handle just NOPs than no > > conversion at all, so I guess making that easy is still good. > > In my view nop_convert is useful to avoid cluttering the code with > (if (tree_nop_conversion_p ...) checks that are even redundant > when a (convert? ... is stripped away. > > > > Like the attached (need to adjust docs & rename some functions still) > > > which generalizes > > > [digit]? parsing. This allows you to write (nop_convert? ...) > > > > I guess once this is in, we should replace all (most?) 'nop_convert' with > > 'nop_convert?' (and possibly a digit in some places) and remove the last > > alternative in the definition of nop_convert. > > Yes, that was my thinking. > > > Although that will increase the code size. In case, we could still have both a > > nop_convert and a strip_nop predicates. Just thinking: > > We should measure it, yes, I hope it won't be too bad ;) In theory > making genmatch emitted code more like a graph rather than a tree > (with shared leafs) might save us a bit here. > > > (match (nop_convert @0) > > (convert @0) > > (if (tree_nop_conversion_p (type, TREE_TYPE (@0))))) > > (match (nop_convert @0) > > (view_convert @0) > > (if (VECTOR_TYPE_P (type) && VECTOR_TYPE_P (TREE_TYPE (@0)) > > && known_eq (TYPE_VECTOR_SUBPARTS (type), > > TYPE_VECTOR_SUBPARTS (TREE_TYPE (@0))) > > && tree_nop_conversion_p (TREE_TYPE (type), TREE_TYPE (TREE_TYPE > > && (@0)))))) > > > > (match (strip_nops @0) > > (nop_convert? @0)) > > > > but that relies on the fact that when there is an optional operation, the > > machinery first tries with the operation, and then without, the order matters. > > Maybe being explicit on the priorities is safer > > > > (match (strip_nops @0) > > (nop_convert @0)) > > (match (strip_nops @0) > > @0) > > Yeah, I don't think the above complexity is worth it. > > > > It works (generated files are unchanged), so I guess I'll push it > > > after polishing. > > > > > > It also extends the 1/2/3 grouping to be able to group like (plus > > > (nop_convert2? @0) (convert2? @1)) > > > so either both will be present or none (meaning that when grouping you > > > can choose the "cheaper" > > > test for one in case you know the conversions will be the same). > > > > Nice. > > r279037. And testing the following now, replacing all nop_converts. > wc -l gimple-match.c.orig 117182 gimple-match.c.orig > wc -l gimple-match.c 119052 gimple-match.c so impact is minimal. Richard. 2019-12-06 Richard Biener <rguenther@suse.de> * match.pd (nop_convert): Remove empty match. Use nop_convert? everywhere. Index: gcc/match.pd =================================================================== --- gcc/match.pd (revision 279037) +++ gcc/match.pd (working copy) @@ -98,8 +98,8 @@ (define_operator_list UNCOND_TERNARY (define_operator_list COND_TERNARY IFN_COND_FMA IFN_COND_FMS IFN_COND_FNMA IFN_COND_FNMS) -/* As opposed to convert?, this still creates a single pattern, so - it is not a suitable replacement for convert? in all cases. */ +/* With nop_convert? combine convert? and view_convert? in one pattern + plus conditionalize on tree_nop_conversion_p conversions. */ (match (nop_convert @0) (convert @0) (if (tree_nop_conversion_p (type, TREE_TYPE (@0))))) @@ -109,9 +109,6 @@ (define_operator_list COND_TERNARY && known_eq (TYPE_VECTOR_SUBPARTS (type), TYPE_VECTOR_SUBPARTS (TREE_TYPE (@0))) && tree_nop_conversion_p (TREE_TYPE (type), TREE_TYPE (TREE_TYPE (@0)))))) -/* This one has to be last, or it shadows the others. */ -(match (nop_convert @0) - @0) /* Transform likes of (char) ABS_EXPR <(int) x> into (char) ABSU_EXPR <x> ABSU_EXPR returns unsigned absolute value of the operand and the operand @@ -1428,7 +1425,7 @@ (define_operator_list COND_TERNARY /* Convert - (~A) to A + 1. */ (simplify - (negate (nop_convert (bit_not @0))) + (negate (nop_convert? (bit_not @0))) (plus (view_convert @0) { build_each_one_cst (type); })) /* Convert ~ (A - 1) or ~ (A + -1) to -A. */ @@ -1455,7 +1452,7 @@ (define_operator_list COND_TERNARY /* Otherwise prefer ~(X ^ Y) to ~X ^ Y as more canonical. */ (simplify - (bit_xor:c (nop_convert:s (bit_not:s @0)) @1) + (bit_xor:c (nop_convert?:s (bit_not:s @0)) @1) (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) (bit_not (bit_xor (view_convert @0) @1)))) @@ -1684,7 +1681,7 @@ (define_operator_list COND_TERNARY /* For equality, this is also true with wrapping overflow. */ (for op (eq ne) (simplify - (op:c (nop_convert@3 (plus:c@2 @0 (convert1? @1))) (convert2? @1)) + (op:c (nop_convert?@3 (plus:c@2 @0 (convert1? @1))) (convert2? @1)) (if (ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0)) && (TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0)) || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0))) @@ -1693,7 +1690,7 @@ (define_operator_list COND_TERNARY && tree_nop_conversion_p (TREE_TYPE (@3), TREE_TYPE (@1))) (op @0 { build_zero_cst (TREE_TYPE (@0)); }))) (simplify - (op:c (nop_convert@3 (pointer_plus@2 (convert1? @0) @1)) (convert2? @0)) + (op:c (nop_convert?@3 (pointer_plus@2 (convert1? @0) @1)) (convert2? @0)) (if (tree_nop_conversion_p (TREE_TYPE (@2), TREE_TYPE (@0)) && tree_nop_conversion_p (TREE_TYPE (@3), TREE_TYPE (@0)) && (CONSTANT_CLASS_P (@1) || (single_use (@2) && single_use (@3)))) @@ -2142,7 +2139,7 @@ (define_operator_list COND_TERNARY || !HONOR_SIGN_DEPENDENT_ROUNDING (type))) (convert (negate @1)))) (simplify - (negate (nop_convert (negate @1))) + (negate (nop_convert? (negate @1))) (if (!TYPE_OVERFLOW_SANITIZED (type) && !TYPE_OVERFLOW_SANITIZED (TREE_TYPE (@1))) (view_convert @1))) @@ -2159,25 +2156,25 @@ (define_operator_list COND_TERNARY /* A - (A +- B) -> -+ B */ /* A +- (B -+ A) -> +- B */ (simplify - (minus (nop_convert (plus:c (nop_convert @0) @1)) @0) + (minus (nop_convert1? (plus:c (nop_convert2? @0) @1)) @0) (view_convert @1)) (simplify - (minus (nop_convert (minus (nop_convert @0) @1)) @0) + (minus (nop_convert1? (minus (nop_convert2? @0) @1)) @0) (if (!ANY_INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_WRAPS (type)) (negate (view_convert @1)) (view_convert (negate @1)))) (simplify - (plus:c (nop_convert (minus @0 (nop_convert @1))) @1) + (plus:c (nop_convert1? (minus @0 (nop_convert2? @1))) @1) (view_convert @0)) (simplify - (minus @0 (nop_convert (plus:c (nop_convert @0) @1))) + (minus @0 (nop_convert1? (plus:c (nop_convert2? @0) @1))) (if (!ANY_INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_WRAPS (type)) (negate (view_convert @1)) (view_convert (negate @1)))) (simplify - (minus @0 (nop_convert (minus (nop_convert @0) @1))) + (minus @0 (nop_convert1? (minus (nop_convert2? @0) @1))) (view_convert @1)) /* (A +- B) + (C - A) -> C +- B */ /* (A + B) - (A - C) -> B + C */ @@ -2204,7 +2201,7 @@ (define_operator_list COND_TERNARY (for inner_op (plus minus) neg_inner_op (minus plus) (simplify - (outer_op (nop_convert (inner_op @0 CONSTANT_CLASS_P@1)) + (outer_op (nop_convert? (inner_op @0 CONSTANT_CLASS_P@1)) CONSTANT_CLASS_P@2) /* If one of the types wraps, use that one. */ (if (!ANY_INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_WRAPS (type)) @@ -2243,7 +2240,7 @@ (define_operator_list COND_TERNARY /* (CST1 - A) +- CST2 -> CST3 - A */ (for outer_op (plus minus) (simplify - (outer_op (nop_convert (minus CONSTANT_CLASS_P@1 @0)) CONSTANT_CLASS_P@2) + (outer_op (nop_convert? (minus CONSTANT_CLASS_P@1 @0)) CONSTANT_CLASS_P@2) /* If one of the types wraps, use that one. */ (if (!ANY_INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_WRAPS (type)) /* If all 3 captures are CONSTANT_CLASS_P, punt, as we might recurse @@ -2262,7 +2259,7 @@ (define_operator_list COND_TERNARY Use view_convert because it is safe for vectors and equivalent for scalars. */ (simplify - (minus CONSTANT_CLASS_P@1 (nop_convert (minus CONSTANT_CLASS_P@2 @0))) + (minus CONSTANT_CLASS_P@1 (nop_convert? (minus CONSTANT_CLASS_P@2 @0))) /* If one of the types wraps, use that one. */ (if (!ANY_INTEGRAL_TYPE_P (type) || TYPE_OVERFLOW_WRAPS (type)) /* If all 3 captures are CONSTANT_CLASS_P, punt, as we might recurse
On Fri, 6 Dec 2019, Richard Biener wrote: >> Although that will increase the code size. In case, we could still have both a >> nop_convert and a strip_nop predicates. Just thinking: > > We should measure it, yes, I hope it won't be too bad ;) In theory > making genmatch emitted code more like a graph rather than a tree > (with shared leafs) might save us a bit here. Indeed, it isn't worth hacking this specific case. If we really want those savings, making it automatic at a higher level is the right way to go. On Fri, 6 Dec 2019, Richard Biener wrote: > @@ -1684,7 +1681,7 @@ (define_operator_list COND_TERNARY > /* For equality, this is also true with wrapping overflow. */ > (for op (eq ne) > (simplify > - (op:c (nop_convert@3 (plus:c@2 @0 (convert1? @1))) (convert2? @1)) > + (op:c (nop_convert?@3 (plus:c@2 @0 (convert1? @1))) (convert2? @1)) > (if (ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0)) > && (TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0)) > || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0))) Possible future clean-up: those convert1 and convert2 have (strange) associated tree_nop_conversion_p below and look like candidates to become nop_convert. > @@ -2159,25 +2156,25 @@ (define_operator_list COND_TERNARY > /* A - (A +- B) -> -+ B */ > /* A +- (B -+ A) -> +- B */ > (simplify > - (minus (nop_convert (plus:c (nop_convert @0) @1)) @0) > + (minus (nop_convert1? (plus:c (nop_convert2? @0) @1)) @0) > (view_convert @1)) I was wondering if we could use nop_convert for both (instead of nop_convert1 and 2), but for constants that wouldn't work, so this looks good.
--- gcc/match.pd.jj 2019-12-03 10:20:00.244913801 +0100 +++ gcc/match.pd 2019-12-03 13:36:01.084435697 +0100 @@ -2159,20 +2159,26 @@ (define_operator_list COND_TERNARY /* A - (A +- B) -> -+ B */ /* A +- (B -+ A) -> +- B */ (simplify - (minus (plus:c @0 @1) @0) - @1) + (minus (nop_convert (plus:c (nop_convert @0) @1)) @0) + (view_convert @1)) (simplify - (minus (minus @0 @1) @0) - (negate @1)) + (minus (nop_convert (minus (nop_convert @0) @1)) @0) + (if (!ANY_INTEGRAL_TYPE_P (type) + || TYPE_OVERFLOW_WRAPS (type)) + (negate (view_convert @1)) + (view_convert (negate @1)))) (simplify - (plus:c (minus @0 @1) @1) - @0) + (plus:c (nop_convert (minus @0 (nop_convert @1))) @1) + (view_convert @0)) (simplify - (minus @0 (plus:c @0 @1)) - (negate @1)) + (minus @0 (nop_convert (plus:c (nop_convert @0) @1))) + (if (!ANY_INTEGRAL_TYPE_P (type) + || TYPE_OVERFLOW_WRAPS (type)) + (negate (view_convert @1)) + (view_convert (negate @1)))) (simplify - (minus @0 (minus @0 @1)) - @1) + (minus @0 (nop_convert (minus (nop_convert @0) @1))) + (view_convert @1)) /* (A +- B) + (C - A) -> C +- B */ /* (A + B) - (A - C) -> B + C */ /* More cases are handled with comparisons. */ --- gcc/testsuite/gcc.dg/tree-ssa/pr92734-2.c.jj 2019-12-03 13:56:05.036800119 +0100 +++ gcc/testsuite/gcc.dg/tree-ssa/pr92734-2.c 2019-12-03 13:55:43.035140773 +0100 @@ -0,0 +1,76 @@ +/* PR tree-optimization/92734 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-optimized" } */ +/* Verify there are no binary additions or subtractions left. There can + be just casts and negations. */ +/* { dg-final { scan-tree-dump-not " \[+-] " "optimized" } } */ + +int +f1 (int x, unsigned y) +{ + int a = x + y; + return a - x; +} + +unsigned +f2 (unsigned x, int y) +{ + unsigned a = (int) x + y; + return a - x; +} + +int +f3 (int x, unsigned y) +{ + int a = x - y; + return a - x; +} + +unsigned +f4 (unsigned x, int y) +{ + unsigned a = (int) x - y; + return a - x; +} + +int +f5 (unsigned x, int y) +{ + int a = x - y; + return a + y; +} + +unsigned +f6 (int x, unsigned y) +{ + unsigned a = x - (int) y; + return a + y; +} + +int +f7 (int x, unsigned y) +{ + int a = x + y; + return x - a; +} + +unsigned +f8 (unsigned x, int y) +{ + unsigned a = (int) x + y; + return x - a; +} + +int +f9 (int x, unsigned y) +{ + int a = x - y; + return x - a; +} + +unsigned +f10 (unsigned x, int y) +{ + unsigned a = (int) x - y; + return x - a; +}