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

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

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 ?

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 ?
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 ?

Thanks,
Prathamesh

[-- Attachment #2: foo.patch --]
[-- Type: text/x-patch, Size: 509 bytes --]

Index: genmatch.c
===================================================================
--- genmatch.c	(revision 223225)
+++ genmatch.c	(working copy)
@@ -3329,7 +3329,7 @@
 		      "others with arity %d", oper, idb->nargs, arity);
 
 	  user_id *p = dyn_cast<user_id *> (idb);
-	  if (p && p->is_oper_list)
+	  if (p && !p->substitutes.is_empty()) // p is either user-defined operator list or a list iterator
 	    op->substitutes.safe_splice (p->substitutes);
 	  else 
 	    op->substitutes.safe_push (idb);

[-- Attachment #3: ChangeLog --]
[-- Type: application/octet-stream, Size: 162 bytes --]

2015-05-16  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	* genmatch.c (parser::parse_for): Insert p->substitutes in op if p->substitues is
	not empty.

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

[Richard Biener]

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.

So - can you instead reject this use?

Thanks,
Richard.

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

[Prathamesh Kulkarni]

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.
>
> So - can you instead reject this use?
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.

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

[Prathamesh Kulkarni]

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.

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 ?

Thanks,
Prathamesh
>>
>> So - can you instead reject this use?
> 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.

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

[Richard Biener]

On Mon, 18 May 2015, Prathamesh Kulkarni 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.
> >
> > So - can you instead reject this use?
> 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.

Hmm, but is this really a useful extension?  To me it just complicates
the syntax for the occasional reader.

Richard.

> 
> 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)

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

[Marek Polacek]

On Tue, May 19, 2015 at 10:33:08AM +0200, Richard Biener wrote:
> > Would that be a good idea ? For symmetry, I thought
> > (for op (list)
> >       op2 (op))
> > should be supported too.
> 
> Hmm, but is this really a useful extension?  To me it just complicates
> the syntax for the occasional reader.

FWIW, I'd prefer this to be rejected than supported.

	Marek

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

[Richard Biener]

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.

> 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.

Richard.

> Thanks,
> Prathamesh
> >>
> >> So - can you instead reject this use?
> > 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)

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 5640 bytes --]

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)

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 595 bytes --]

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);
 	}

[-- Attachment #3: ChangeLog.txt --]
[-- Type: text/plain, Size: 144 bytes --]

2015-05-20  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

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

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

[Richard Biener]

On Wed, 20 May 2015, Prathamesh Kulkarni wrote:

> 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 ?

Ok.

Thanks,
Richard.

> 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)
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)