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

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

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:
>> On February 9, 2015 11:09:49 AM CET, Bin Cheng <bin.cheng@arm.com> wrote:
>>
>> Did you extract a testcase for it?  Note that the IV step itself may be expanded
>> Too much.
>>
>>   I
>>>looked into the regression and thought it was because of passes after
>>>IVOPT.
>>>The IVOPT dump is at least not worse than the original version.
>>
>> But different I presume.  So how did you figure it regressed?
> The tree level dump is like,
>
> ---    Without patch
> +++ With patch
>    <bb 8>:
>    _618 = MAX_EXPR <n_105, 0>;
>    _619 = (integer(kind=8)) _618;
>    _629 = (unsigned long) _618;
>
>    ......
>
>    <bb 37>:
>    _257 = _619 + 1;
>    _255 = (sizetype) _257;
>    _261 = _255 * 8;
>    _256 = (sizetype) _140;
> -  _1174 = (sizetype) _618;
> + _1174 = (sizetype) _619;
>    _1175 = _256 + _1174;
>    _1176 = _1175 * 8;
>    _1177 = _148 + _1176;
>    ivtmp.3849_258 = (unsigned long) _1177;
>    _1179 = (unsigned int) _104;
>
> 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).

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

Thanks,
Richard.

> Thanks,
> bin
>>
>> Thanks,
>> Richard.
>>
>>>Bootstrap and test on x86_64 and AArch64, so is it OK?
>>>
>>>2015-02-09  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_simplify_using_condition_1, refine_bounds_using_guard)
>>>       (number_of_iterations_exit): Pass new argument to
>>>       expand_simple_operations.
>>>       * 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.  Call extract_single_var_from_expr.
>>>       (difference_cannot_overflow_p): Pass new argument to
>>>       expand_simple_operations.
>>>
>>>gcc/testsuite/ChangeLog
>>>2015-02-09  Bin Cheng  <bin.cheng@arm.com>
>>>
>>>       PR tree-optimization/64705
>>>       * gcc.dg/tree-ssa/pr64705.c: New test.
>>
>>