Re: [PATCH PR64705]Don't aggressively expand induction variable's base

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Feb 12, 2015 at 8:28 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Wed, Feb 11, 2015 at 7:24 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Feb 11, 2015 at 9:23 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>>> On Tue, Feb 10, 2015 at 12:24 AM, Richard Biener
>>> <richard.guenther@gmail.com> wrote:
>>>
>>> Previously, the computation of _1174 can be replaced by _629 in bb8 in
>>> DOM2 pass, while it can't after patching.  This is the only possible
>>> regression that I can see on TREE level.  There is another difference
>>> but not regression on TREE.  Seems real change happens on RTL pre with
>>> different register uses in the input.  I failed to go further or
>>> extract a test case, it's just very volatile.
>>
>> Well, I can see what is happening and indeed we shouldn't blame the
>> patch for this.
>>
>>> With all of this, I guess this patch shouldn't be blamed for this.  I
>>> also wonder if the PR should be fixed in this way since the patch
>>> definitely is a corner case.
>>
>> It might not fix the real issue (whatever that is), but not making
>> IVOPTs (or tree-affines) life harder is very good (I believe I have
>> seen this kind of issue as well).
> I guess IV's base is expanded because we want to explore more CSE opportunities?
> I suspect this doesn't work very well because of two reasons: 1)
> overload the tree-affine facility; 2) weak IV rewriting capacity in
> GCC (for example, mess up loop variant/invariant part expressions).  I
> will do experiments on this.
>
> As for the patch itself, I collected instrumental data from GCC
> bootstrap and Spec2k6 compilation.  Can confirm in most cases
> (bootstrap 99.9%, spec2k6 99.1%), there is only one ssa name in IV's
> step.
>
>>
>> So I do think that the patch is fine.  Just seen the known-to-work GCC 3.4
>> version so it's even a regression ....
>
> Here is the refined patch according to your comments.  It passes
> bootstrap and test on x86_64.

Ok.

Thanks,
Richard.

> Thanks,
> bin
>
> 2015-02-12  Bin Cheng  <bin.cheng@arm.com>
>
>     PR tree-optimization/64705
>     * tree-ssa-loop-niter.h (expand_simple_operations): New parameter.
>     * tree-ssa-loop-niter.c (expand_simple_operations): New parameter.
>     * tree-ssa-loop-ivopts.c (extract_single_var_from_expr): New.
>     (find_bivs, find_givs_in_stmt_scev): Pass new argument to
>     expand_simple_operations.
>
> gcc/testsuite/ChangeLog
> 2015-02-12  Bin Cheng  <bin.cheng@arm.com>
>
>     PR tree-optimization/64705
>     * gcc.dg/tree-ssa/pr64705.c: New test.