This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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 <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)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]