[Bug tree-optimization/59417] [4.9 Regression] ICE in determine_value_range, at tree-ssa-loop-niter.c:176

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59417

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #2)
> The problem is the uninitialized var t in the testcase, with it apparently
> the range info weird and inconsistent, but what sanity can one expect from
> uninitialized value, any use of it is invalid.
> 
> Before *.copyprop5 we have:
>   # RANGE [1, 2147483647] NONZERO 0x0000000007fffffff
>   # t_4 = PHI <t_19(D)(3), t_8(23)>
> (supposedly because we ignore the uninitialized var in some spots).
> Then during copyprop5 we copy the range info of t_4 to t_19(D):
>           else if (!POINTER_TYPE_P (TREE_TYPE (var))
>                    && SSA_NAME_RANGE_INFO (var)
>                    && !SSA_NAME_RANGE_INFO (copy_of[i].value))
>             duplicate_ssa_name_range_info (copy_of[i].value,
>                                            SSA_NAME_RANGE_TYPE (var),
>                                            SSA_NAME_RANGE_INFO (var));
> I'm wondering how that can be a safe thing to do, even when not taking into
> account undefined vars.  If we have say:
>   if (parm_5(D) < 32)
>     {
>       do_something_with (parm_5(D));
>       goto return;
>     }
>   somevar_21 = parm_5(D);
> and somevar_21 will have range info of [32, +INF] (from VRP ASSERT_EXPRs),
> then
> I don't see how it can be safe to modify parm_5(D)'s SSA_NAME_RANGE_INFO
> (similarly for pointer alignment info though).  For this testcase, surely we
> could say avoid doing it if the copy_of[i].value SSA_NAME is default def,
> but I think it is a general issue.  Perhaps points to info can be copied
> over, but not alignment info or range info.  Maybe we could consider not
> doing copyprop or forwprop replacing one SSA_NAME with another one if the
> one to be replaced has better range info (or alignment info) and only
> copyprop/forwprop if we would replace SSA_NAME with something other than
> SSA_NAME?

Yeah, I wondered about this as well after reviewing the original patches.
If you consider

  a_2 = a_1;
  if (a_2 > 5)
    a_3 = a_2;

then VRP would say a_3 is [6, +INF].  If then copyprop comes along it
sees that a_3 = a_2 = a_1 and would transfer the value-range to a_1.

Note that the loop doing that may replace SSA annotation multiple times
if copy_of[N].value == X for multiple N.

Basically the code relies on the information being not control sensitive.
That's fine for points-to info (but alignment tracking now uses nonzero_bits?),
but for the rest only if var is post-dominated by copy_of[].value's definition.

I don't think we should limit what copyprop/forwprop does though.

Now, why do we arrive at a value-range where we fail that assertion?

> Then in tree-ssa-loop-niter.c I can surely instead of assetion failure just
> give up on using the range info altogether (rtype = VR_VARYING; break;) or
> just using var's range info and nothing else if there is inconsistency
> (rtype = get_range_info (var, &minv, &maxv); break;).