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.
> 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 <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham
Norton, HRB 21284 (AG Nuernberg)
