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

[Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>]

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.