On Wed, Nov 9, 2016 at 3:30 PM, Dominik Vogt <v...@linux.vnet.ibm.com> wrote:
> 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.gli...@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 <v...@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 <v...@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?
No, just do not match SSA_NAME. And instead of
+ (with { gimple *def_stmt = SSA_NAME_DEF_STMT (@0); }
+ (if (is_gimple_assign (def_stmt)
+ && CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def_stmt)))
you instead want to change the pattern to
(simpify
(cmp (convert @0) INTEGER_CST@1)
@0 will then be your innerop
note that you can't use get_value_range but you have to use the
get_range_info interface instead. I suppose a helper function
somewhere that computes whether an expression fits a type
would be helpful (see expr_not_equal_to for sth related,
thus expr_fits_type_p (@0, TREE_TYPE (@1)))
Likewise the overflow_infinity checks do not translate to match.pd
(or rahter the range info you get).
Richard.
>
> 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
