| Message ID | CAAgBjMn1VJO6aMXzLNZfzG1WnzjyuZkn2gkHJd1gnpDLf+HqVA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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? Thanks, Richard.
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.
On Mon, 18 May 2015, Prathamesh Kulkarni 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. > > > > 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. Hmm, but is this really a useful extension? To me it just complicates the syntax for the occasional reader. Richard. > > Thanks, > Prathamesh > > > > > > Thanks, > > Richard. > >
On Tue, May 19, 2015 at 10:33:08AM +0200, Richard Biener wrote: > > Would that be a good idea ? For symmetry, I thought > > (for op (list) > > op2 (op)) > > should be supported too. > > Hmm, but is this really a useful extension? To me it just complicates > the syntax for the occasional reader. FWIW, I'd prefer this to be rejected than supported. Marek
On Tue, 19 May 2015, Prathamesh Kulkarni wrote: > 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. 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. > >
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