On Wed, 20 May 2015, Prathamesh Kulkarni wrote: > 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 ?
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 <rguent...@suse.de> > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, > > Graham Norton, HRB 21284 (AG Nuernberg) > -- Richard Biener <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)
