phiopt: Adjust instead of reset phires range

  On Thu, Dec 22, 2022 at 01:09:21PM +0100, Aldy Hernandez wrote:
>  INTEGER_CST singleton and
> > union that into the SSA_NAMEs range and then do set_range_info
> > with the altered range I guess.
> >
> 
> Note that set_range_info is an intersect operation. It should really be
> called update_range_info. Up to now, there were no users that wanted to
> clobber old ranges completely.

Thanks.
That would be then (I've committed the previous patch, also for reasons of
backporting) following incremental patch.

For the just committed testcase, it does the right thing,
# RANGE [irange] int [-INF, -1][1, +INF]
# iftmp.2_9 = PHI <iftmp.3_10(4), 8(5)>
is before the range (using -fdump-tree-all-alias) and r below is
[irange] int [-INF, -1][1, +INF],
unioned with carg of 0 into VARYING.
If I try attached testcase though (which just uses signed char d instead of
int d to give more interesting range info), then I see:
# RANGE [irange] int [-128, -1][1, 127]
# iftmp.2_10 = PHI <iftmp.3_11(4), 8(5)>
but strangely r I get from range_of_expr is
[irange] int [-128, 127]
rather than the expected [irange] int [-128, -1][1, 127].
Sure, it is later unioned with 0, so it doesn't change anything, but I
wonder what is the difference.  Note, this is before actually replacing
the phi arg 8(5) with iftmp.3_11(5).
At that point bb4 is:
<bb 4> [local count: 966367640]:
# RANGE [irange] int [-128, 127]
# iftmp.3_11 = PHI <iftmp.3_15(3), 0(2)>
if (iftmp.3_11 != 0)
  goto <bb 6>; [56.25%]
else
  goto <bb 5>; [43.75%]
and bb 5 is empty forwarder, so [-128, -1][1, 127] is actually correct.
Either iftmp.3_11 is non-zero, then iftmp.2_10 is that value and its range, or
it is zero and then iftmp.2_10 is 8, so [-128, -1][1, 127] U [8, 8], but
more importantly SSA_NAME_RANGE_INFO should be at least according to what
is printed be without 0.

2022-12-22  Jakub Jelinek  <jakub@redhat.com>
	    Aldy Hernandez  <aldyh@redhat.com>

	* tree-ssa-phiopt.cc (value_replacement): Instead of resetting
	phires range info, union it with oarg.



	Jakub
// PR tree-optimization/108166
// { dg-do run }

bool a, b;
signed char d;
int c;

const int &
foo (const int &f, const int &g)
{
  return !f ? f : g;
}

__attribute__((noipa)) void
bar (int)
{
}

int
main ()
{
  c = foo (b, 0) > ((b ? d : b) ?: 8);
  a = b ? d : b;
  bar (a);
  if (a != 0)
    __builtin_abort ();
}
  

On Thu, Dec 22, 2022 at 1:54 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Thu, Dec 22, 2022 at 01:09:21PM +0100, Aldy Hernandez wrote:
> >  INTEGER_CST singleton and
> > > union that into the SSA_NAMEs range and then do set_range_info
> > > with the altered range I guess.
> > >
> >
> > Note that set_range_info is an intersect operation. It should really be
> > called update_range_info. Up to now, there were no users that wanted to
> > clobber old ranges completely.
>
> Thanks.
> That would be then (I've committed the previous patch, also for reasons of
> backporting) following incremental patch.
>
> For the just committed testcase, it does the right thing,
> # RANGE [irange] int [-INF, -1][1, +INF]
> # iftmp.2_9 = PHI <iftmp.3_10(4), 8(5)>
> is before the range (using -fdump-tree-all-alias) and r below is
> [irange] int [-INF, -1][1, +INF],
> unioned with carg of 0 into VARYING.
> If I try attached testcase though (which just uses signed char d instead of
> int d to give more interesting range info), then I see:
> # RANGE [irange] int [-128, -1][1, 127]
> # iftmp.2_10 = PHI <iftmp.3_11(4), 8(5)>
> but strangely r I get from range_of_expr is
> [irange] int [-128, 127]
> rather than the expected [irange] int [-128, -1][1, 127].
> Sure, it is later unioned with 0, so it doesn't change anything, but I
> wonder what is the difference.  Note, this is before actually replacing
> the phi arg 8(5) with iftmp.3_11(5).
> At that point bb4 is:
> <bb 4> [local count: 966367640]:
> # RANGE [irange] int [-128, 127]
> # iftmp.3_11 = PHI <iftmp.3_15(3), 0(2)>
> if (iftmp.3_11 != 0)
>   goto <bb 6>; [56.25%]
> else
>   goto <bb 5>; [43.75%]
> and bb 5 is empty forwarder, so [-128, -1][1, 127] is actually correct.
> Either iftmp.3_11 is non-zero, then iftmp.2_10 is that value and its range, or
> it is zero and then iftmp.2_10 is 8, so [-128, -1][1, 127] U [8, 8], but
> more importantly SSA_NAME_RANGE_INFO should be at least according to what
> is printed be without 0.
>
> 2022-12-22  Jakub Jelinek  <jakub@redhat.com>
>             Aldy Hernandez  <aldyh@redhat.com>
>
>         * tree-ssa-phiopt.cc (value_replacement): Instead of resetting
>         phires range info, union it with oarg.
>
> --- gcc/tree-ssa-phiopt.cc.jj   2022-12-22 12:52:36.588469821 +0100
> +++ gcc/tree-ssa-phiopt.cc      2022-12-22 13:11:51.145060050 +0100
> @@ -1492,11 +1492,25 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
>                     break;
>                   }
>               if (equal_p)
> -               /* After the optimization PHI result can have value
> -                  which it couldn't have previously.
> -                  We could instead of resetting it union the range
> -                  info with oarg.  */
> -               reset_flow_sensitive_info (gimple_phi_result (phi));
> +               {
> +                 tree phires = gimple_phi_result (phi);
> +                 if (SSA_NAME_RANGE_INFO (phires))
> +                   {
> +                     /* After the optimization PHI result can have value
> +                        which it couldn't have previously.  */
> +                     value_range r;

I haven't looked at your problem above, but have you tried using
int_range_max (or even int_range<2>) instead of value_range above?

value_range is deprecated and uses the legacy anti-range business,
which has a really hard time representing complex ranges, as well as
union/intersecting them.

> +                     if (get_global_range_query ()->range_of_expr (r, phires,
> +                                                                   phi))
> +                       {
> +                         int_range<2> tmp (carg, carg);
> +                         r.union_ (tmp);

Here you are taking the legacy value_range and unioning into it.
That's bound to lose precision.

Ideally you should use int_range_max for intermediate calculations.
Then set_range_info() will take care of squishing things down into
whatever we allow into a global range (I think it's a 6-sub range
object ??).

Note, that if "r" can contain non integer/pointers (i.e. floats), you
should use:

  // Range of <TYPE>.
  Value_Range r (<TYPE>);

The goal is for Value_Range to become value_range, and for it to be
used for anything not explicitly an integer/pointer.  Thus the camel
case for this release.

Aldy

> +                         reset_flow_sensitive_info (phires);
> +                         set_range_info (phires, r);
> +                       }
> +                     else
> +                       reset_flow_sensitive_info (phires);
> +                   }
> +               }
>               if (equal_p && MAY_HAVE_DEBUG_BIND_STMTS)
>                 {
>                   imm_use_iterator imm_iter;
>
>
>         Jakub