On 19 May 2015 at 14:34, Richard Biener <rguent...@suse.de> wrote:
> On Tue, 19 May 2015, Prathamesh Kulkarni wrote:
>
>> On 18 May 2015 at 20:17, Prathamesh Kulkarni
>> <prathamesh.kulka...@linaro.org> wrote:
>> > On 18 May 2015 at 14:12, Richard Biener <rguent...@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 <rguent...@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham
> Norton, HRB 21284 (AG Nuernberg)
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);
}
2015-05-20 Prathamesh Kulkarni <prathamesh.kulka...@linaro.org>
* genmatch.c (parser::parse_for): Reject iterator if used as
operator-list.
