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.