[PATCH] phiopt: Drop SSA_NAME_RANGE_INFO in maybe equal case [PR108166]

[PATCH] phiopt: Drop SSA_NAME_RANGE_INFO in maybe equal case [PR108166]

[Jakub Jelinek]

Hi!

The following place in value_replacement is after proving that
x == cst1 ? cst2 : x
phi result is only used in a comparison with constant which doesn't
care if it compares cst1 or cst2 and replaces it with x.
The testcase is miscompiled because we have after the replacement
incorrect range info for the phi result, we would need to
effectively union the phi result range with cst1 (oarg in the code)
because previously that constant might be missing in the range, but
newly it can appear (we've just verified that the single use stmt
of the phi result doesn't care about that value in particular).

The following patch just resets the info, bootstrapped/regtested
on x86_64-linux and i686-linux, ok for trunk?

Aldy/Andrew, how would one instead union the SSA_NAME_RANGE_INFO
with some INTEGER_CST and store it back into SSA_NAME_RANGE_INFO
(including adjusting non-zero bits and the like)?

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

	PR tree-optimization/108166
	* tree-ssa-phiopt.cc (value_replacement): For the maybe_equal_p
	case turned into equal_p reset SSA_NAME_RANGE_INFO of phi result.

	* g++.dg/torture/pr108166.C: New test.

--- gcc/tree-ssa-phiopt.cc.jj	2022-10-28 11:00:53.970243821 +0200
+++ gcc/tree-ssa-phiopt.cc	2022-12-21 14:27:58.118326548 +0100
@@ -1491,6 +1491,12 @@ value_replacement (basic_block cond_bb,
 		  default:
 		    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));
 	      if (equal_p && MAY_HAVE_DEBUG_BIND_STMTS)
 		{
 		  imm_use_iterator imm_iter;
--- gcc/testsuite/g++.dg/torture/pr108166.C.jj	2022-12-21 14:31:02.638661322 +0100
+++ gcc/testsuite/g++.dg/torture/pr108166.C	2022-12-21 14:30:45.441909725 +0100
@@ -0,0 +1,26 @@
+// PR tree-optimization/108166
+// { dg-do run }
+
+bool a, b;
+int d, 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 ();
+}

	Jakub

Re: [PATCH] phiopt: Drop SSA_NAME_RANGE_INFO in maybe equal case [PR108166]

[Aldy Hernandez]

On Thu, Dec 22, 2022 at 11:30 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> The following place in value_replacement is after proving that
> x == cst1 ? cst2 : x
> phi result is only used in a comparison with constant which doesn't
> care if it compares cst1 or cst2 and replaces it with x.
> The testcase is miscompiled because we have after the replacement
> incorrect range info for the phi result, we would need to
> effectively union the phi result range with cst1 (oarg in the code)
> because previously that constant might be missing in the range, but
> newly it can appear (we've just verified that the single use stmt
> of the phi result doesn't care about that value in particular).
>
> The following patch just resets the info, bootstrapped/regtested
> on x86_64-linux and i686-linux, ok for trunk?
>
> Aldy/Andrew, how would one instead union the SSA_NAME_RANGE_INFO
> with some INTEGER_CST and store it back into SSA_NAME_RANGE_INFO
> (including adjusting non-zero bits and the like)?

if (get_global_range_query ()->range_of_expr (r, <SSA>, <STMT>)) {
  int_range<2> tmp (<INTEGER_CST>, <INTEGER_CST>);
  r.union_ (tmp);
  set_range_info (<SSA>, r);
}

Note that the <STMT> is unused, as the global range doesn't have
context.  But it is good form to pass it since we could decide at a
later time to replace get_global_range_query() with a context-aware
get_range_query(), or a ranger instance.  But <STMT> is not strictly
necessary.

Hmmm, set_range_info() is an intersect operation, because we always
update what's already there, never replace it.  If want to replace the
global range, throwing away whatever range we had there, you may want
to call this first:

/* Reset all flow sensitive data on NAME such as range-info, nonzero
   bits and alignment.  */

void
reset_flow_sensitive_info (tree name)
{
}

Aldy

>
> 2022-12-22  Jakub Jelinek  <jakub@redhat.com>
>
>         PR tree-optimization/108166
>         * tree-ssa-phiopt.cc (value_replacement): For the maybe_equal_p
>         case turned into equal_p reset SSA_NAME_RANGE_INFO of phi result.
>
>         * g++.dg/torture/pr108166.C: New test.
>
> --- gcc/tree-ssa-phiopt.cc.jj   2022-10-28 11:00:53.970243821 +0200
> +++ gcc/tree-ssa-phiopt.cc      2022-12-21 14:27:58.118326548 +0100
> @@ -1491,6 +1491,12 @@ value_replacement (basic_block cond_bb,
>                   default:
>                     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));
>               if (equal_p && MAY_HAVE_DEBUG_BIND_STMTS)
>                 {
>                   imm_use_iterator imm_iter;
> --- gcc/testsuite/g++.dg/torture/pr108166.C.jj  2022-12-21 14:31:02.638661322 +0100
> +++ gcc/testsuite/g++.dg/torture/pr108166.C     2022-12-21 14:30:45.441909725 +0100
> @@ -0,0 +1,26 @@
> +// PR tree-optimization/108166
> +// { dg-do run }
> +
> +bool a, b;
> +int d, 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 ();
> +}
>
>         Jakub
>

Re: [PATCH] phiopt: Drop SSA_NAME_RANGE_INFO in maybe equal case [PR108166]

[Richard Biener]

On Thu, 22 Dec 2022, Jakub Jelinek wrote:

> Hi!
> 
> The following place in value_replacement is after proving that
> x == cst1 ? cst2 : x
> phi result is only used in a comparison with constant which doesn't
> care if it compares cst1 or cst2 and replaces it with x.
> The testcase is miscompiled because we have after the replacement
> incorrect range info for the phi result, we would need to
> effectively union the phi result range with cst1 (oarg in the code)
> because previously that constant might be missing in the range, but
> newly it can appear (we've just verified that the single use stmt
> of the phi result doesn't care about that value in particular).
> 
> The following patch just resets the info, bootstrapped/regtested
> on x86_64-linux and i686-linux, ok for trunk?

OK.

> Aldy/Andrew, how would one instead union the SSA_NAME_RANGE_INFO
> with some INTEGER_CST and store it back into SSA_NAME_RANGE_INFO
> (including adjusting non-zero bits and the like)?

There's no get_range_info on SSA_NAMEs (anymore?) but you can
construct a value_range from the INTEGER_CST singleton and
union that into the SSA_NAMEs range and then do set_range_info
with the altered range I guess.

Richard.

> 2022-12-22  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/108166
> 	* tree-ssa-phiopt.cc (value_replacement): For the maybe_equal_p
> 	case turned into equal_p reset SSA_NAME_RANGE_INFO of phi result.
> 
> 	* g++.dg/torture/pr108166.C: New test.
> 
> --- gcc/tree-ssa-phiopt.cc.jj	2022-10-28 11:00:53.970243821 +0200
> +++ gcc/tree-ssa-phiopt.cc	2022-12-21 14:27:58.118326548 +0100
> @@ -1491,6 +1491,12 @@ value_replacement (basic_block cond_bb,
>  		  default:
>  		    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));
>  	      if (equal_p && MAY_HAVE_DEBUG_BIND_STMTS)
>  		{
>  		  imm_use_iterator imm_iter;
> --- gcc/testsuite/g++.dg/torture/pr108166.C.jj	2022-12-21 14:31:02.638661322 +0100
> +++ gcc/testsuite/g++.dg/torture/pr108166.C	2022-12-21 14:30:45.441909725 +0100
> @@ -0,0 +1,26 @@
> +// PR tree-optimization/108166
> +// { dg-do run }
> +
> +bool a, b;
> +int d, 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 ();
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] phiopt: Drop SSA_NAME_RANGE_INFO in maybe equal case [PR108166]

[Aldy Hernandez]

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

On Thu, Dec 22, 2022, 12:33 Richard Biener <rguenther@suse.de> wrote:

> On Thu, 22 Dec 2022, Jakub Jelinek wrote:
>
> > Hi!
> >
> > The following place in value_replacement is after proving that
> > x == cst1 ? cst2 : x
> > phi result is only used in a comparison with constant which doesn't
> > care if it compares cst1 or cst2 and replaces it with x.
> > The testcase is miscompiled because we have after the replacement
> > incorrect range info for the phi result, we would need to
> > effectively union the phi result range with cst1 (oarg in the code)
> > because previously that constant might be missing in the range, but
> > newly it can appear (we've just verified that the single use stmt
> > of the phi result doesn't care about that value in particular).
> >
> > The following patch just resets the info, bootstrapped/regtested
> > on x86_64-linux and i686-linux, ok for trunk?
>
> OK.
>
> > Aldy/Andrew, how would one instead union the SSA_NAME_RANGE_INFO
> > with some INTEGER_CST and store it back into SSA_NAME_RANGE_INFO
> > (including adjusting non-zero bits and the like)?
>
> There's no get_range_info on SSA_NAMEs (anymore?) but you can
> construct a value_range from the


This is my fault. When we added get_global_range_query, I removed
get_range_info. I should've left that entry point. I'll look into adding it
next cycle for readability's sake


 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.

Aldy


> Richard.
>
> > 2022-12-22  Jakub Jelinek  <jakub@redhat.com>
> >
> >       PR tree-optimization/108166
> >       * tree-ssa-phiopt.cc (value_replacement): For the maybe_equal_p
> >       case turned into equal_p reset SSA_NAME_RANGE_INFO of phi result.
> >
> >       * g++.dg/torture/pr108166.C: New test.
> >
> > --- gcc/tree-ssa-phiopt.cc.jj 2022-10-28 11:00:53.970243821 +0200
> > +++ gcc/tree-ssa-phiopt.cc    2022-12-21 14:27:58.118326548 +0100
> > @@ -1491,6 +1491,12 @@ value_replacement (basic_block cond_bb,
> >                 default:
> >                   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));
> >             if (equal_p && MAY_HAVE_DEBUG_BIND_STMTS)
> >               {
> >                 imm_use_iterator imm_iter;
> > --- gcc/testsuite/g++.dg/torture/pr108166.C.jj        2022-12-21
> 14:31:02.638661322 +0100
> > +++ gcc/testsuite/g++.dg/torture/pr108166.C   2022-12-21
> 14:30:45.441909725 +0100
> > @@ -0,0 +1,26 @@
> > +// PR tree-optimization/108166
> > +// { dg-do run }
> > +
> > +bool a, b;
> > +int d, 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 ();
> > +}
> >
> >       Jakub
> >
> >
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> HRB 36809 (AG Nuernberg)
>
>

[PATCH] phiopt: Adjust instead of reset phires range

[Jakub Jelinek]

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

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

[-- Attachment #2: pr108166-2.C --]
[-- Type: text/plain, Size: 318 bytes --]

// 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 ();
}

Re: [PATCH] phiopt: Adjust instead of reset phires range

[Aldy Hernandez]

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

[PATCH] phiopt, v2: Adjust instead of reset phires range

[Jakub Jelinek]

On Thu, Dec 22, 2022 at 08:46:33PM +0100, Aldy Hernandez wrote:
> 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.

You're right.  With int_range_max it works as I expected.
And no, floating point isn't possible here.
If value_range is right now just the legacy single range or anti-range,
then it explains why it didn't work - while on the first testcase
we could have anti-range ~[0, 0], on the second case [-128, -1] U [1, 127]
is turned into simple legacy [-128, 127].

So, ok for trunk if this passes bootstrap/regtest?

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,
 		    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.  */
+		      int_range_max r;
+		      if (get_global_range_query ()->range_of_expr (r, phires,
+								    phi))
+			{
+			  int_range<2> tmp (carg, carg);
+			  r.union_ (tmp);
+			  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

Re: [PATCH] phiopt, v2: Adjust instead of reset phires range

[Aldy Hernandez]

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

LGTM

On Thu, Dec 22, 2022, 22:44 Jakub Jelinek <jakub@redhat.com> wrote:

> On Thu, Dec 22, 2022 at 08:46:33PM +0100, Aldy Hernandez wrote:
> > 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.
>
> You're right.  With int_range_max it works as I expected.
> And no, floating point isn't possible here.
> If value_range is right now just the legacy single range or anti-range,
> then it explains why it didn't work - while on the first testcase
> we could have anti-range ~[0, 0], on the second case [-128, -1] U [1, 127]
> is turned into simple legacy [-128, 127].
>
> So, ok for trunk if this passes bootstrap/regtest?
>
> 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,
>                     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.  */
> +                     int_range_max r;
> +                     if (get_global_range_query ()->range_of_expr (r,
> phires,
> +                                                                   phi))
> +                       {
> +                         int_range<2> tmp (carg, carg);
> +                         r.union_ (tmp);
> +                         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
>
>