| Message ID | CAAgBjMn1VJO6aMXzLNZfzG1WnzjyuZkn2gkHJd1gnpDLf+HqVA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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);
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