This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern
- From: Richard Biener <rguenther at suse dot de>
- To: Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>
- Cc: gcc Patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 20 May 2015 10:31:17 +0200 (CEST)
- Subject: Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern
- Authentication-results: sourceware.org; auth=none
- References: <CAAgBjMn1VJO6aMXzLNZfzG1WnzjyuZkn2gkHJd1gnpDLf+HqVA at mail dot gmail dot com> <alpine dot LSU dot 2 dot 11 dot 1505181033170 dot 18702 at zhemvz dot fhfr dot qr> <CAAgBjM=DN08AncZcgcwGXhaWvyFCADj3FEQF2NYVpu-ZWhGjfQ at mail dot gmail dot com> <CAAgBjMnL9k74HO3Y4paXGGU+B=t1k4BrChipBiTC717hwg7Keg at mail dot gmail dot com> <alpine dot LSU dot 2 dot 11 dot 1505191102430 dot 18702 at zhemvz dot fhfr dot qr> <CAAgBjMnHE5eYT9=-HWNCDrj9mdaNZaE-u-0nY+ZAxHC8vOnX+g at mail dot gmail dot com>
On Wed, 20 May 2015, Prathamesh Kulkarni wrote:
> On 19 May 2015 at 14:34, Richard Biener <rguenther@suse.de> wrote:
> > 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.
> 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 <rguenther@suse.de>
> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)