Re: [RFC] Check number of uses in simplify_cond_using_ranges().

[Dominik Vogt <vogt at linux dot vnet dot ibm dot com>]

On Wed, Nov 09, 2016 at 01:43:58PM +0100, Richard Biener wrote:
> On Tue, Nov 8, 2016 at 5:18 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> > On Tue, 8 Nov 2016, Dominik Vogt wrote:
> >
> >> On Fri, Nov 04, 2016 at 01:54:20PM +0100, Richard Biener wrote:
> >>>
> >>> On Fri, Nov 4, 2016 at 12:08 PM, Dominik Vogt <vogt@linux.vnet.ibm.com>
> >>> wrote:
> >>>>
> >>>> On Fri, Nov 04, 2016 at 09:47:26AM +0100, Richard Biener wrote:
> >>>>>
> >>>>> On Thu, Nov 3, 2016 at 4:03 PM, Dominik Vogt <vogt@linux.vnet.ibm.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> Is VRP the right pass to do this optimisation or should a later
> >>>>>> pass rather attempt to eliminate the new use of b_5 instead?  Uli
> >>>>>> has brought up the idea a mini "sign extend elimination" pass that
> >>>>>> checks if the result of a sign extend could be replaced by the
> >>>>>> original quantity in all places, and if so, eliminate the ssa
> >>>>>> name.  (I guess that won't help with the above code because l is
> >>>>>> used also as a function argument.)  How could a sensible approach
> >>>>>> to deal with the situation look like?
> >>>>>
> >>>>>
> >>>>> We run into this kind of situation regularly and for general foldings
> >>>>> in match.pd we settled with single_use () even though it is not
> >>>>> perfect.
> >>>>> Note the usual complaint is not extra extension instructions but
> >>>>> the increase of register pressure.
> >>>>>
> >>>>> This is because it is hard to do better when you are doing local
> >>>>> optimization.
> >>>>>
> >>>>> As for the question on whether VRP is the right pass to do this the
> >>>>> answer is two-fold -- VRP has the most precise range information.
> >>>>> But the folding itself should be moved to generic code and use
> >>>>> get_range_info ().
> >>>>
> >>>>
> >>>> All right, is this a sensible approach then?
> >>>
> >>>
> >>> Yes.
> >>>
> >>>>   1. Using has_single_use as in the experimental patch is good
> >>>>      enough (provided further testing does not show serious
> >>>>      regressions).
> >>>
> >>>
> >>> I'd approve that, yes.
> >>>
> >>>>   2. Rip the whole top level if-block from simplify_cond_using_ranges().
> >>>>   3. Translate that code to match.pd syntax.
> >>>
> >>>
> >>> Might be some work but yes, that's also desired (you'd lose the ability
> >>> to emit the warnings though).
> >>
> >>
> >> Could you give me a match-pd-hint please?  We now have something
> >> like this:
> >>
> >> (simplify
> >>  (cond (gt SSA_NAME@0 INTEGER_CST@1) @2 @3)
> >>  (if (... many conditions ...)
> >>   (cond (gt ... ...) @2 @3))
> >>
> >> The generated code ends up in gimple_simplify_COND_EXPR, but when
> >> gimple_simplify is actually called, it goes through the
> >> GIMPLE_COND case and calls gimple_resimplify2(..., GT, ...) and
> >> there it tries gimple_simplify_GT_EXPR(), peeling of the (cond
> >> ...), i.e. it never tries the generated code.
> >
> >
> > Not sure what you mean here.
> >
> >> There is another pattern in match.pd that uses a (cond ...) as the
> >> first operand, and I do not understand how this works.  Should we
> >> just use "(gt SSA_NAME@0 INTEGER_CST@1)" as the first operand
> >> instead, and wouldn't this pattern be too general that way?
> >
> >
> > IIUC, you are trying to move the second half of simplify_cond_using_ranges
> > to match.pd. I don't see any reason to restrict it to the case where the
> > comparison result is used directly in a COND_EXPR, so that would look like:
> >
> > (for cmp (...)
> >  (simplify
> >   (cmp (convert SSA_NAME@0) INTEGER_CST@1)
> >   (if (...)
> >    (cmp @0 (convert @1)))))
> >
> > maybe? I think I may have missed your point.
> 
> Yeah, if you'd use (cond (gt ... then it only matches in assignments
> with COND_EXPRs on the RHS, _not_ in GIMPLE_CONDs.
> 
> So you ran into the (cond vs. GIMPLE_COND "mismatch".
> 
> You'd essentially implement sth similar to shorten_compare in match.pd.

Something like the attached patch?  Robin and me have spent quite
some time to figure out the new pattern.  Two questions:

1) In the match expression you cannot just use SSA_NAME@0 because
   then the "case SSA_NAME:" is added to a switch for another
   pattern that already has that label.  Thus we made that "proxy"
   predicate "ssa_name_p" that forces the code for the new pattern
   out of the old switch and into a separate code block.  We
   couldn't figure out whether this joining of case labels is a
   feature in the matching language.  So, is this the right way to
   deal with the conflicting labels?

2) There's an awful lot of if-clauses in the replacement
   expression.  Is it the right place to do all this checking?

> Btw, moving to match.pd shouldn't be a blocker for adding proper
> single_use tests
> just in case you get lost ...

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Attachment: 0001-Convert-folding-in-simplify_cond_using_ranges-to-mat.patch
Description: Text document