Re: [PATCH PR41488]Recognize more induction variables by simplifying PEELED chrec in scalar evolution

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Mon, Dec 9, 2013 at 12:00 PM, Jeff Law <law@redhat.com> wrote:
> On 12/06/13 19:44, Bin.Cheng wrote:
>>>
>>> Right.  Based on reading the archives, it looks like this stuff is/was
>>> generated by PRE.  I also suspect jump threading can create them.  There
>>> was
>>> talk of throttling PRE to leave things in a form that the IV analysis
>>> could
>>> more easily digest, but I'm not sure if that was ever completed.
>>
>>
>> Also could just because it is coded in that way, just as the case I
>> added in patch.  I found real examples from ggc-page.c in GCC.
>> But it's always good to cleanup input of an optimization, I presume
>> that's why Richard tried to move IVOPT later before.
>
> Certainly.  That possibility was mentioned as well in either 41488 or
> elsewhere in my research.
>
>
>>>
>>> I assume tree_to_aff_combination_expand is relatively expensive, thus the
>>> two approaches, one which avoids tree_to_aff_combination_expand.
>>
>> Considering the dump of case in the patch:
>
> [ snip ]
> So it's also a case using the affine stuff gets you get a simpler interface
> to express the value in terms you may be able match.
Yeah.

>
>
>>
>> Another question, is it acceptable to add an parameter for
>> tree_to_aff_combination_expand to limit the number of recursive call
>> for it?  Thus we don't need to expand to leaf node every time.
>
> If there's a compelling reason to limit the recursion, then I'd think such a
> parameter would be reasonable.
Not for now.  Just come up this idea given that some recent patches
also use the interface to get back trace information and it's
expensive.  Maybe Richard have some comment here too.

>
>
>
>>
>>>
>>>
>>> In add_old_iv_candidates, is it the case that the only time
>>> SSA_NAME_DEF_STMT (def) is another PHI node is when it's one of these
>>> affine
>>
>>
>> I suppose.  Actually IVOPT make an assert on IP_ORIGINAL candidates in
>> function rewrite_use_nonlinear_expr, like:
>
> Well, the reason I ask is after your patch we'd be ignoring anything where
> SSA_NAME_DEF_STMT (def) is a PHI node and not a PEELED_CHREC.  I want to
> make sure there aren't other expected cases where SSA_NAME_DEF_STMT (def) is
> a PHI that we'd want to call add_candidate_1 for.
>
> It sounds like there aren't other cases you'd expect to see here where
> SSA_NAME_DEF_STMT (defFFF) is a PHI.
Yes.

>
>
>> IVOPT adds original cand and tries to keep it the original IV is
>> because it does have an updating statement.  For IVs picked up by this
>> patch, it doesn't have stepping statement, so no need/way to leaving
>> it untouched.  It will be represented by candidates for IVs from which
>> it is peeled.
>
> Understood.  Thanks for clarifying.  The patch is fine for the trunk.
Thanks very much for reviewing.  Since Sebastian hasn't added any
comments, I will change the patch as suggested by Richard before, then
retest it, and check in the new version if it's ok.

Thanks,
bin

>
> jeff



-- 
Best Regards.