diff mbox

[match-and-simplify] fix incorrect code-gen in 'for' pattern

Message ID CAAgBjMnHE5eYT9=-HWNCDrj9mdaNZaE-u-0nY+ZAxHC8vOnX+g@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni May 19, 2015, 9:39 p.m. UTC 
On 19 May 2015 at 14:34, Richard Biener <rguenther@suse.de> wrote:
> On Tue, 19 May 2015, Prathamesh Kulkarni wrote:
>
>> On 18 May 2015 at 20:17, Prathamesh Kulkarni
>> <prathamesh.kulkarni@linaro.org> wrote:
>> > On 18 May 2015 at 14:12, Richard Biener <rguenther@suse.de> wrote:
>> >> On Sat, 16 May 2015, Prathamesh Kulkarni wrote:
>> >>
>> >>> Hi,
>> >>> genmatch generates incorrect code for following (artificial) pattern:
>> >>>
>> >>> (for op (plus)
>> >>>       op2 (op)
>> >>>   (simplify
>> >>>     (op @x @y)
>> >>>     (op2 @x @y)
>> >>>
>> >>> generated gimple code: http://pastebin.com/h1uau9qB
>> >>> 'op' is not replaced in the generated code on line 33:
>> >>> *res_code = op;
>> >>>
>> >>> I think it would be a better idea to make op2 iterate over same set
>> >>> of operators (op2->substitutes = op->substitutes).
>> >>> I have attached patch for the same.
>> >>> Bootstrap + testing in progress on x86_64-unknown-linux-gnu.
>> >>> OK for trunk after bootstrap+testing completes ?
>> >>
>> >> Hmm, but then the example could as well just use 'op'.  I think we
>> >> should instead reject this.
>> >>
>> >> Consider
>> >>
>> >>   (for op (plus minus)
>> >>     (for op2 (op)
>> >>       (simplify ...
>> >>
>> >> where it is not clear what would be desired.  Simple replacement
>> >> of 'op's value would again just mean you could have used 'op' in
>> >> the first place.  Doing what you propose would get you
>> >>
>> >>   (for op (plus minus)
>> >>     (for op2 (plus minus)
>> >>       (simplify ...
>> >>
>> >> thus a different iteration.
>> >>
>> >>> I wonder if we really need is_oper_list flag in user_id ?
>> >>> We can determine if user_id is an operator list
>> >>> if user_id::substitutes is not empty ?
>> >>
>> >> After your change yes.
>> >>
>> >>> That will lose the ability to distinguish between user-defined operator
>> >>> list and list-iterator in for like op/op2, but I suppose we (so far) don't
>> >>> need to distinguish between them ?
>> >>
>> >> Well, your change would simply make each list-iterator a (temporary)
>> >> user-defined operator list as well as the current iterator element
>> >> (dependent on context - see the nested for example).  I think that
>> >> adds to confusion.
>> AFAIU, the way it's implemented in lower_for, the iterator is handled
>> the same as a user-defined operator
>> list. I was wondering if we should get rid of 'for' altogether and
>> have it replaced
>> by operator-list ?
>>
>> IMHO having two different things - iterator and operator-list is
>> unnecessary and we could
>> brand iterator as a "local" operator-list. We could extend syntax of 'simplify'
>> to accommodate "local" operator-lists.
>>
>> So we can say, using an operator-list within 'match' replaces it by
>> corresponding operators in that list.
>> Operator-lists can be "global" (visible to all patterns), or local to
>> a particular pattern.
>>
>> eg:
>> a) single for
>> (for op (...)
>>   (simplify
>>     (op ...)))
>>
>> can be written as:
>> (simplify
>>   op (...)  // define "local" operator-list op.
>>   (op ...)) // proceed here the same way as for lowering "global" operator list.
>
> it's not shorter and it's harder to parse.  And you can't share the
> operator list with multiple simplifies like
>
>  (for op (...)
>    (simplify
>      ...)
>    (simplify
>      ...))
>
> which is already done I think.
I missed that -;)
Well we can have a "workaround syntax" for that if desired.
>
>> b) multiple iterators:
>> (for op1 (...)
>>       op2 (...)
>>   (simplify
>>     (op1 (op2 ...))))
>>
>> can be written as:
>> (simplify
>>   op1 (...)
>>   op2 (...)
>>   (op1 (op2 ...)))
>>
>> c) nested for
>> (for op1 (...)
>>     (for op2 (...)
>>       (simplify
>>         (op1 (op2 ...))))
>>
>> can be written as:
>>
>> (simplify
>>   op1 (...)
>>   (simplify
>>     op2 (...)
>>     (op1 (op2 ...))))
>>
>> My rationale behind removing 'for' is we don't need to distinguish
>> between an "operator-list" and "iterator",
>> and only have an operator-list -;)
>> Also we can reuse parser::parse_operator_list (in parser::parse_for
>> parsing oper-list is duplicated)
>> and get rid of 'parser::parse_for'.
>> We don't need to change lowering, since operator-lists are handled
>> the same way as 'for' (we can keep lowering of simplify::for_vec as it is).
>>
>> Does it sound reasonable ?
>
> I dont' think the proposed syntax is simpler or more powerful.
Hmm I tend to agree. My motivation to remove 'for' was that it is
not more powerful than operator-list and we can re-write 'for' with equivalent
operator-list with some syntax changes (like putting operator-list in
simplify etc.)
So there's only one of doing the same thing.

>
> Richard.
>
>> Thanks,
>> Prathamesh
>> >>
>> >> So - can you instead reject this use?
I have attached patch for rejecting this use of iterator.
Ok for trunk after bootstrap+testing ?

Thanks,
Prathamesh
>> > Well my intention was to have support for walking operator list in reverse.
>> > For eg:
>> > (for bitop (bit_and bit_ior)
>> >       rbitop (bit_ior bit_and)
>> >    ...)
>> > Could be replaced by sth like:
>> > (for bitop (bit_and bit_ior)
>> >       rbitop (~bitop))
>> >    ...)
>> >
>> > where "~bitop" would indicate walking (bit_and bit_ior) in reverse.
>> > Would that be a good idea ? For symmetry, I thought
>> > (for op (list)
>> >       op2 (op))
>> > should be supported too.
>> >
>> >
>> > Thanks,
>> > Prathamesh
>> >
>> >
>> >>
>> >> Thanks,
>> >> Richard.
>>
>>
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)
2015-05-20  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	* genmatch.c (parser::parse_for): Reject iterator if used as operator-list.
diff mbox

Patch

Index: genmatch.c
===================================================================
--- genmatch.c	(revision 223352)
+++ genmatch.c	(working copy)
@@ -3329,8 +3329,13 @@ 
 		      "others with arity %d", oper, idb->nargs, arity);
 
 	  user_id *p = dyn_cast<user_id *> (idb);
-	  if (p && p->is_oper_list)
-	    op->substitutes.safe_splice (p->substitutes);
+	  if (p)
+	    {
+	      if (p->is_oper_list)
+		op->substitutes.safe_splice (p->substitutes);
+	      else
+		fatal_at (token, "iterator cannot be used as operator-list");
+	    }
 	  else 
 	    op->substitutes.safe_push (idb);
 	}