Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern
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
Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern
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
Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern
On Mon, 18 May 2015, Prathamesh Kulkarni 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. 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. -- Richard Biener rguent...@suse.de SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)
Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern
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)
Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern
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
Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern
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. 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.
Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern
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.
Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern
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. 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.
