[RFA,tree-optimization/78496] 01/03 Do not lose range information from earlier VRP passes

As we touched on in IRC, the EVRP analyzer was doing something stupid
which caused it to not merge in existing range information under certain
circumstances.

Consider this fragment:

  x_1 = foo ()
  if (x_1 > 2)
    __builtin_unreachable ();
  if (x_1 < 0)
    __builtin_unreachable ();

Obviously the range for x_1 is [0,2] and we compute that range in the
EVRP optimization pass as well as VRP.


If a pass (say VRP) were to delete the __builtin_unreachable calls we'll
be left with:


  x_1 = foo ()

Any subsequent EVRP analysis won't be able to generate range information
for that statement -- ie, it looks like VR_VARYING.  Due to a dumb bug
in the EVRP analysis we allowed that VR_VARYING to override any range
that had been computed by an earlier VRP or EVRP pass.


Fixing is trivial.  Always call update_value_range, even if the
currently discovered range is VR_VARYING.

Bootstrapped and regression tested, both in isolation and as part of
this 3 part kit.

OK for the trunk?

Jeff
* gimple-ssa-evrp-analyze.c
	(evrp_range_analyzer::extract_range_from_stmt):  Always use
	vr_values::update_value_range so preexisting range info is
	medged with new range info, even if the new range is VR_VARYING.

On Sun, Dec 03, 2017 at 10:55:27PM -0700, Jeff Law wrote:
> As we touched on in IRC, the EVRP analyzer was doing something stupid
> which caused it to not merge in existing range information under certain
> circumstances.
> 
> Consider this fragment:
> 
>   x_1 = foo ()
>   if (x_1 > 2)
>     __builtin_unreachable ();
>   if (x_1 < 0)
>     __builtin_unreachable ();

Note that for say:
  x_1 = foo ();
  bar (x_1);
  if (x_1 > 2)
    __builtin_unreachable ();
  if (x_1 < 0)
    __builtin_unreachable ();
  ...
  further uses of x_1
we can't do that anymore (at least, can't remember it in
SSA_NAME_RANGE_INFO), as bar could not return/could loop etc.  Ditto with
any other code in between foo and the unreachable asserts if it doesn't
guarantee that we'll always reach the comparisons after the x_1 setter.
Even
  x_1 = foo ();
  bar ();
  if (x_1 > 2)
    __builtin_unreachable ();
  if (x_1 < 0)
    __builtin_unreachable ();
looks unclean, if bar doesn't return, then we'd just need to hope we don't
add further uses of x_1 in between foo and bar.  Some optimizations do stuff
like that, consider foo being a pass-through function.

	Jakub

On Mon, Dec 4, 2017 at 6:55 AM, Jeff Law <law@redhat.com> wrote:
> As we touched on in IRC, the EVRP analyzer was doing something stupid
> which caused it to not merge in existing range information under certain
> circumstances.
>
> Consider this fragment:
>
>   x_1 = foo ()
>   if (x_1 > 2)
>     __builtin_unreachable ();
>   if (x_1 < 0)
>     __builtin_unreachable ();
>
> Obviously the range for x_1 is [0,2] and we compute that range in the
> EVRP optimization pass as well as VRP.
>
> If a pass (say VRP) were to delete the __builtin_unreachable calls we'll
> be left with:
>
>
>   x_1 = foo ()
>
> Any subsequent EVRP analysis won't be able to generate range information
> for that statement -- ie, it looks like VR_VARYING.  Due to a dumb bug
> in the EVRP analysis we allowed that VR_VARYING to override any range
> that had been computed by an earlier VRP or EVRP pass.

Doh - probably not noticed because EVRP was only run "first" ...

>
> Fixing is trivial.  Always call update_value_range, even if the
> currently discovered range is VR_VARYING.
>
> Bootstrapped and regression tested, both in isolation and as part of
> this 3 part kit.
>
> OK for the trunk?

Ok.

Thanks,
Richard.

> Jeff
>
>         * gimple-ssa-evrp-analyze.c
>         (evrp_range_analyzer::extract_range_from_stmt):  Always use
>         vr_values::update_value_range so preexisting range info is
>         medged with new range info, even if the new range is VR_VARYING.
>
> diff --git a/gcc/gimple-ssa-evrp-analyze.c b/gcc/gimple-ssa-evrp-analyze.c
> index 551b1d6..fb3d329 100644
> --- a/gcc/gimple-ssa-evrp-analyze.c
> +++ b/gcc/gimple-ssa-evrp-analyze.c
> @@ -271,8 +271,7 @@ evrp_range_analyzer::record_ranges_from_stmt (gimple *stmt)
>        edge taken_edge;
>        value_range vr = VR_INITIALIZER;
>        vr_values->extract_range_from_stmt (stmt, &taken_edge, &output, &vr);
> -      if (output
> -         && (vr.type == VR_RANGE || vr.type == VR_ANTI_RANGE))
> +      if (output)
>         {
>           vr_values->update_value_range (output, &vr);
>
>

On 12/04/2017 01:11 AM, Jakub Jelinek wrote:
> On Sun, Dec 03, 2017 at 10:55:27PM -0700, Jeff Law wrote:
>> As we touched on in IRC, the EVRP analyzer was doing something stupid
>> which caused it to not merge in existing range information under certain
>> circumstances.
>>
>> Consider this fragment:
>>
>>   x_1 = foo ()
>>   if (x_1 > 2)
>>     __builtin_unreachable ();
>>   if (x_1 < 0)
>>     __builtin_unreachable ();
> 
> Note that for say:
>   x_1 = foo ();
>   bar (x_1);
>   if (x_1 > 2)
>     __builtin_unreachable ();
>   if (x_1 < 0)
>     __builtin_unreachable ();
>   ...
>   further uses of x_1
> we can't do that anymore (at least, can't remember it in
> SSA_NAME_RANGE_INFO), as bar could not return/could loop etc.
Right.  Anything reflected into SSA_NAME_RANGE_INFO has to be globally
true.  With the call to bar the transformation can't safely be applied.

  Ditto with
> any other code in between foo and the unreachable asserts if it doesn't
> guarantee that we'll always reach the comparisons after the x_1 setter.
> Even
>   x_1 = foo ();
>   bar ();
>   if (x_1 > 2)
>     __builtin_unreachable ();
>   if (x_1 < 0)
>     __builtin_unreachable ();
> looks unclean, if bar doesn't return, then we'd just need to hope we don't
> add further uses of x_1 in between foo and bar.  Some optimizations do stuff
> like that, consider foo being a pass-through function.
This one is less clear.  But I don't think we should be trying to
optimize this case anyway -- too little to be gained and too close to
doing something unexpected.

jeff