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

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

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

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

[-- Attachment #2: P1 --]
[-- Type: text/plain, Size: 765 bytes --]

	* 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);
 

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

[Jakub Jelinek]

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

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

[Richard Biener]

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);
>
>

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

[Jeff Law]

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