diff mbox

[match-and-simplify] support operator list

Message ID CAJXstsDUf6C93KQUyYsLKpd-qiZHy83FjEvsgFCmzjzuW1SGhw@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni Nov. 3, 2014, 12:56 p.m. UTC 
(had sent it earlier by private mail).

The attached patch supports operator-list and it's use in for.
For now, operator-list is rejected in expression.

This patch also allows user-defined operator to be used as operator-list
(user-defined ops are really temporary or "scoped" operator-lists).
(for op (plus minus)
      op2 (op...)
  (simplify
     ...))

Should that be supported or rejected ?

* genmatch.c
  (user_id): Add new member is_oper_list.
  (user_id::user_id): Add new default argument.
  (parser::parse_operator_list): New function.
  (parser::parse_for): Allow operator-list.
  (parser::parse_pattern): Call parser::parse_operator_list.
  (parser::parse_operation): Reject operator-list.

* match-comparison.pd
  Define operator-lists eq_ops and cc and use them in patterns.

Thanks,
Prathamesh

Comments

Jakub Jelinek Nov. 3, 2014, 1:01 p.m. UTC | #1 
On Mon, Nov 03, 2014 at 06:26:12PM +0530, Prathamesh Kulkarni wrote:
> --- gcc/match-comparison.pd	(revision 216916)
> +++ gcc/match-comparison.pd	(working copy)
> @@ -1,5 +1,8 @@
>  /* From fold_binary.  */
>  
> +(define_operator_list eq_ops eq ne)
> +(define_operator_list cc eq_ops lt le gt ge)

I think cc is a bad name for the macro, that usually stands for condition
code register.

	Jakub
Richard Biener Nov. 3, 2014, 3:46 p.m. UTC | #2 
On Mon, Nov 3, 2014 at 2:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Nov 03, 2014 at 06:26:12PM +0530, Prathamesh Kulkarni wrote:
>> --- gcc/match-comparison.pd   (revision 216916)
>> +++ gcc/match-comparison.pd   (working copy)
>> @@ -1,5 +1,8 @@
>>  /* From fold_binary.  */
>>
>> +(define_operator_list eq_ops eq ne)
>> +(define_operator_list cc eq_ops lt le gt ge)
>
> I think cc is a bad name for the macro, that usually stands for condition
> code register.

OTOH it is a perfect match for 'condition code'.

Richard.

>         Jakub
Jakub Jelinek Nov. 3, 2014, 3:50 p.m. UTC | #3 
On Mon, Nov 03, 2014 at 04:46:43PM +0100, Richard Biener wrote:
> On Mon, Nov 3, 2014 at 2:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Mon, Nov 03, 2014 at 06:26:12PM +0530, Prathamesh Kulkarni wrote:
> >> --- gcc/match-comparison.pd   (revision 216916)
> >> +++ gcc/match-comparison.pd   (working copy)
> >> @@ -1,5 +1,8 @@
> >>  /* From fold_binary.  */
> >>
> >> +(define_operator_list eq_ops eq ne)
> >> +(define_operator_list cc eq_ops lt le gt ge)
> >
> > I think cc is a bad name for the macro, that usually stands for condition
> > code register.
> 
> OTOH it is a perfect match for 'condition code'.

So eqcodes and ccodes, or comp_code, ... ?
Saving a few keystrokes there can be a problem for readability.
Not to mention that there are various other tcc_comparison codes (lggt,
unordered, ordered, un{lt,le,gt,ge,eq}).

	Jakub
Richard Biener Nov. 4, 2014, 1:01 p.m. UTC | #4 
On Mon, Nov 3, 2014 at 4:50 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Nov 03, 2014 at 04:46:43PM +0100, Richard Biener wrote:
>> On Mon, Nov 3, 2014 at 2:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Mon, Nov 03, 2014 at 06:26:12PM +0530, Prathamesh Kulkarni wrote:
>> >> --- gcc/match-comparison.pd   (revision 216916)
>> >> +++ gcc/match-comparison.pd   (working copy)
>> >> @@ -1,5 +1,8 @@
>> >>  /* From fold_binary.  */
>> >>
>> >> +(define_operator_list eq_ops eq ne)
>> >> +(define_operator_list cc eq_ops lt le gt ge)
>> >
>> > I think cc is a bad name for the macro, that usually stands for condition
>> > code register.
>>
>> OTOH it is a perfect match for 'condition code'.
>
> So eqcodes and ccodes, or comp_code, ... ?
> Saving a few keystrokes there can be a problem for readability.
> Not to mention that there are various other tcc_comparison codes (lggt,
> unordered, ordered, un{lt,le,gt,ge,eq}).

Sure.  Let's use ccodes then - or tcc_comparison (though I thought that
was quite long).

Well, the patch should be mostly about the new syntax of course.

Richard.

>
>         Jakub
Richard Biener Nov. 4, 2014, 2:32 p.m. UTC | #5 
On Mon, Nov 3, 2014 at 1:56 PM, Prathamesh Kulkarni
<bilbotheelffriend@gmail.com> wrote:
> (had sent it earlier by private mail).
>
> The attached patch supports operator-list and it's use in for.
> For now, operator-list is rejected in expression.

Ok.

> This patch also allows user-defined operator to be used as operator-list
> (user-defined ops are really temporary or "scoped" operator-lists).
> (for op (plus minus)
>       op2 (op...)
>   (simplify
>      ...))
>
> Should that be supported or rejected ?

I've made it supported, but not in the way you implemented it.  Instead
I am only expanding operator lists.  Thus

(for op (plus minus)
 (for op2 (minus op)
  ...

will iterate (op, op2) (plus, minus) (minus, minus) (plus, plus) (minus, minus).

I think that makes more sense(?) but it's maybe too confusing as well so
eventually we should reject this.

I've adapted the patch slightly, using safe_splice, adding a comment and
avoiding a long line.

I've also used an example operator list in match-builtin.pd instead where
it shows the obvious improvement we're going to implement.

Committed.

Thanks,
Richard.

2014-11-04  Prathamesh Kulkarni  <bilbotheelffriend@gmail.com>

        * genmatch.c (user_id): Add new member is_oper_list.
        (user_id::user_id): Add new default argument.
        (parser::parse_operator_list): New function.
        (parser::parse_for): Allow operator-list.
        (parser::parse_pattern): Call parser::parse_operator_list.
        (parser::parse_operation): Reject operator-list.
        * match-builtin.pd: Define operator lists POWs, CBRTs and SQRTs.

(gmail is too stupid to attach stuff it seems)

> * genmatch.c
>   (user_id): Add new member is_oper_list.
>   (user_id::user_id): Add new default argument.
>   (parser::parse_operator_list): New function.
>   (parser::parse_for): Allow operator-list.
>   (parser::parse_pattern): Call parser::parse_operator_list.
>   (parser::parse_operation): Reject operator-list.
>
> * match-comparison.pd
>   Define operator-lists eq_ops and cc and use them in patterns.
>
> Thanks,
> Prathamesh
diff mbox

Patch

Index: gcc/genmatch.c
===================================================================
--- gcc/genmatch.c	(revision 216916)
+++ gcc/genmatch.c	(working copy)
@@ -234,10 +234,11 @@ 
 
 struct user_id : public id_base
 {
-  user_id (const char *id_)
-    : id_base (id_base::USER, id_), substitutes (vNULL), used (false) {}
+  user_id (const char *id_, bool is_oper_list_ = false)
+    : id_base (id_base::USER, id_), substitutes (vNULL), used (false), is_oper_list (is_oper_list_) {}
   vec<id_base *> substitutes;
   bool used;
+  bool is_oper_list;
 };
 
 template<>
@@ -2443,6 +2444,7 @@ 
   void parse_for (source_location);
   void parse_if (source_location);
   void parse_predicates (source_location);
+  void parse_operator_list ();
 
   cpp_reader *r;
   vec<if_or_with> active_ifs;
@@ -2611,6 +2613,11 @@ 
   id_base *op = get_operator (id);
   if (!op)
     fatal_at (id_tok, "unknown operator %s", id);
+
+  user_id *p = dyn_cast<user_id *> (op);
+  if (p && p->is_oper_list)
+    fatal_at (id_tok, "operator-list not allowed in expression");
+
   return op;
 }
 
@@ -2989,7 +2996,12 @@ 
 	    fatal_at (token, "operator '%s' with arity %d does not match "
 		      "others with arity %d", oper, idb->nargs, arity);
 
-	  op->substitutes.safe_push (idb);
+	  if (user_id *p = dyn_cast<user_id *> (idb))
+	    for (unsigned si = 0; si < p->substitutes.length (); ++si)
+	      op->substitutes.safe_push (p->substitutes[si]);
+	  else 
+	    op->substitutes.safe_push (idb);
+
 	}
       op->nargs = arity;
       token = expect (CPP_CLOSE_PAREN);
@@ -3045,6 +3057,52 @@ 
     }
 }
 
+void
+parser::parse_operator_list ()
+{
+  const cpp_token *token = peek (); 
+  const char *id = get_ident ();
+
+  if (get_operator (id) != 0)
+    fatal_at (token, "operator %s already defined", id);
+
+  user_id *op = new user_id (id, true);
+  int arity = -1;
+  
+  while ((token = peek_ident ()) != 0)
+    {
+      token = peek (); 
+      const char *oper = get_ident ();
+      id_base *idb = get_operator (oper);
+      
+      if (idb == 0)
+	fatal_at (token, "no such operator '%s'", oper);
+
+      if (arity == -1)
+	arity = idb->nargs;
+      else if (idb->nargs == -1)
+	;
+      else if (arity != idb->nargs)
+	fatal_at (token, "operator '%s' with arity %d does not match "
+			 "others with arity %d", oper, idb->nargs, arity);
+
+      if (user_id *p = dyn_cast<user_id *> (idb)) // we don't need to check for ->is_oper_list here.
+	for (unsigned i = 0; i < p->substitutes.length (); ++i)
+	  op->substitutes.safe_push (p->substitutes[i]);
+      else
+	op->substitutes.safe_push (idb);
+    }
+
+  if (op->substitutes.length () == 0)
+    fatal_at (token, "operator-list cannot be empty");
+
+  op->nargs = arity;
+  id_base **slot = operators->find_slot_with_hash (op, op->hashval, INSERT);
+  *slot = op;
+}
+
+
+
 /* Parse an outer if expression.
      if = '(' 'if' '(' <c-expr> ')' <pattern> ')'  */
 
@@ -3145,6 +3203,13 @@ 
 	fatal_at (token, "define_predicates inside if or for is not supported");
       parse_predicates (token->src_loc);
     }
+  else if (strcmp (id, "define_operator_list") == 0)
+    {
+      if (active_ifs.length () > 0
+	  || active_fors.length () > 0)
+	fatal_at (token, "operator-list cannot be defined inside 'for' or 'if'");
+      parse_operator_list ();
+    }
   else
     fatal_at (token, "expected %s'simplify', 'match', 'for' or 'if'",
 	      active_ifs.length () == 0 && active_fors.length () == 0
Index: gcc/match-comparison.pd
===================================================================
--- gcc/match-comparison.pd	(revision 216916)
+++ gcc/match-comparison.pd	(working copy)
@@ -1,5 +1,8 @@ 
 /* From fold_binary.  */
 
+(define_operator_list eq_ops eq ne)
+(define_operator_list cc eq_ops lt le gt ge)
+
 /* On GIMPLE bool != 0 is simply the canonical way to express a
    condition in COND_EXPRs and GIMPLE_CONDs.
    ???  Of course for assignments we still may want to strip those...  */
@@ -15,7 +18,7 @@ 
     @0)))
 
 /* Distribute operations in equality compares.  */
-(for op (eq ne)
+(for op (eq_ops)
  /* -exp op CST is exp op -CST.  */
  (simplify
   /* ??? fix fold-const to use negate_expr_p  */
@@ -29,7 +32,7 @@ 
 /* From fold_comparison, in the order of transforms in there.  */
 
 /* Transform comparisons of the form X +- C1 CMP C2 to X CMP C2 -+ C1.  */
-(for cmp (lt le eq ge gt ne)
+(for cmp (cc)
  (for op (plus minus)
   (simplify
    (cmp (op @0 INTEGER_CST@1) INTEGER_CST@2)
@@ -47,7 +50,7 @@ 
    is undefined for the type, but performing it here badly interacts
    with the transformation in fold_cond_expr_with_comparison which
    attempts to synthetize ABS_EXPR.  */
-(for cmp (eq ne)
+(for cmp (eq_ops) 
  (simplify
   (cmp (minus @0 @1) integer_zerop)
   (cmp @0 @1)))
@@ -65,7 +68,7 @@ 
 #endif
 
 /* Simplify X * C1 CMP 0 to X CMP 0 if C1 is not zero.  */
-(for op (lt le eq ne ge gt)
+(for op (cc)
   (simplify
     (op (mult @0 INTEGER_CST@1) integer_zerop@2)
     /* In fold-const.c we have this and the following pattern
@@ -131,13 +134,13 @@ 
 #endif
 
 /* Fold ~X op ~Y as Y op X.  */
-(for cmp (lt le eq ge gt ne)
+(for cmp (cc)
  (simplify
   (cmp (bit_not @0) (bit_not @1))
   (cmp @1 @0)))
 
 /* Fold ~X op C as X op' ~C, where op' is the swapped comparison.  */
-(for cmp (lt le eq ge gt ne)
+(for cmp (cc)
  (simplify
   (cmp (bit_not @0) @1)
   /* ???  (for cst in INTEGER_CST VECTOR_CST) is not supported yet.  */