diff mbox series

Read global value/mask in IPA.

Message ID 20230717131411.330650-1-aldyh@redhat.com
State New
Headers show
Series Read global value/mask in IPA. | expand

Commit Message

Aldy Hernandez July 17, 2023, 1:14 p.m. UTC 
Instead of reading the known zero bits in IPA, read the value/mask
pair which is available.

There is a slight change of behavior here.  I have removed the check
for SSA_NAME, as the ranger can calculate the range and value/mask for
INTEGER_CST.  This simplifies the code a bit, since there's no special
casing when setting the jfunc bits.  The default range for VR is
undefined, so I think it's safe just to check for undefined_p().

OK?

gcc/ChangeLog:

	* ipa-prop.cc (ipa_compute_jump_functions_for_edge): Read global
	value/mask.
---
 gcc/ipa-prop.cc | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Aldy Hernandez July 18, 2023, 5:37 p.m. UTC | #1 
On 7/17/23 15:14, Aldy Hernandez wrote:
> Instead of reading the known zero bits in IPA, read the value/mask
> pair which is available.
> 
> There is a slight change of behavior here.  I have removed the check
> for SSA_NAME, as the ranger can calculate the range and value/mask for
> INTEGER_CST.  This simplifies the code a bit, since there's no special
> casing when setting the jfunc bits.  The default range for VR is
> undefined, so I think it's safe just to check for undefined_p().

Final round of tests revealed a regression for which I've adjusted the 
testcase.

It turns out g++.dg/ipa/pure-const-3.C fails because IPA can now pick up 
value/mask from any pass that has an integrated ranger.  The test was 
previously disabling evrp and CCP, but now VRP[12], jump threading, and 
DOM can make value/mask adjustments visible to IPA so they must be 
disabled as well.

We've run into these scenarios multiple times in the past-- any 
improvements to the ranger pipeline causes everyone to get smarter, 
making changes visible earlier in the pipeline.

Aldy
Aldy Hernandez July 25, 2023, 6:32 a.m. UTC | #2 
Ping

On Mon, Jul 17, 2023, 15:14 Aldy Hernandez <aldyh@redhat.com> wrote:

> Instead of reading the known zero bits in IPA, read the value/mask
> pair which is available.
>
> There is a slight change of behavior here.  I have removed the check
> for SSA_NAME, as the ranger can calculate the range and value/mask for
> INTEGER_CST.  This simplifies the code a bit, since there's no special
> casing when setting the jfunc bits.  The default range for VR is
> undefined, so I think it's safe just to check for undefined_p().
>
> OK?
>
> gcc/ChangeLog:
>
>         * ipa-prop.cc (ipa_compute_jump_functions_for_edge): Read global
>         value/mask.
> ---
>  gcc/ipa-prop.cc | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
> index 5d790ff1265..4f6ed7b89bd 100644
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> @@ -2402,8 +2402,7 @@ ipa_compute_jump_functions_for_edge (struct
> ipa_func_body_info *fbi,
>         }
>        else
>         {
> -         if (TREE_CODE (arg) == SSA_NAME
> -             && param_type
> +         if (param_type
>               && Value_Range::supports_type_p (TREE_TYPE (arg))
>               && Value_Range::supports_type_p (param_type)
>               && irange::supports_p (TREE_TYPE (arg))
> @@ -2422,15 +2421,14 @@ ipa_compute_jump_functions_for_edge (struct
> ipa_func_body_info *fbi,
>             gcc_assert (!jfunc->m_vr);
>         }
>
> -      if (INTEGRAL_TYPE_P (TREE_TYPE (arg))
> -         && (TREE_CODE (arg) == SSA_NAME || TREE_CODE (arg) ==
> INTEGER_CST))
> +      if (INTEGRAL_TYPE_P (TREE_TYPE (arg)) && !vr.undefined_p ())
>         {
> -         if (TREE_CODE (arg) == SSA_NAME)
> -           ipa_set_jfunc_bits (jfunc, 0,
> -                               widest_int::from (get_nonzero_bits (arg),
> -                                                 TYPE_SIGN (TREE_TYPE
> (arg))));
> -         else
> -           ipa_set_jfunc_bits (jfunc, wi::to_widest (arg), 0);
> +         irange &r = as_a <irange> (vr);
> +         irange_bitmask bm = r.get_bitmask ();
> +         signop sign = TYPE_SIGN (TREE_TYPE (arg));
> +         ipa_set_jfunc_bits (jfunc,
> +                             widest_int::from (bm.value (), sign),
> +                             widest_int::from (bm.mask (), sign));
>         }
>        else if (POINTER_TYPE_P (TREE_TYPE (arg)))
>         {
> --
> 2.40.1
>
>
Aldy Hernandez July 31, 2023, 2:40 p.m. UTC | #3 
PING * 2

On Tue, Jul 25, 2023 at 8:32 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> Ping
>
> On Mon, Jul 17, 2023, 15:14 Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>> Instead of reading the known zero bits in IPA, read the value/mask
>> pair which is available.
>>
>> There is a slight change of behavior here.  I have removed the check
>> for SSA_NAME, as the ranger can calculate the range and value/mask for
>> INTEGER_CST.  This simplifies the code a bit, since there's no special
>> casing when setting the jfunc bits.  The default range for VR is
>> undefined, so I think it's safe just to check for undefined_p().
>>
>> OK?
>>
>> gcc/ChangeLog:
>>
>>         * ipa-prop.cc (ipa_compute_jump_functions_for_edge): Read global
>>         value/mask.
>> ---
>>  gcc/ipa-prop.cc | 18 ++++++++----------
>>  1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
>> index 5d790ff1265..4f6ed7b89bd 100644
>> --- a/gcc/ipa-prop.cc
>> +++ b/gcc/ipa-prop.cc
>> @@ -2402,8 +2402,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
>>         }
>>        else
>>         {
>> -         if (TREE_CODE (arg) == SSA_NAME
>> -             && param_type
>> +         if (param_type
>>               && Value_Range::supports_type_p (TREE_TYPE (arg))
>>               && Value_Range::supports_type_p (param_type)
>>               && irange::supports_p (TREE_TYPE (arg))
>> @@ -2422,15 +2421,14 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
>>             gcc_assert (!jfunc->m_vr);
>>         }
>>
>> -      if (INTEGRAL_TYPE_P (TREE_TYPE (arg))
>> -         && (TREE_CODE (arg) == SSA_NAME || TREE_CODE (arg) == INTEGER_CST))
>> +      if (INTEGRAL_TYPE_P (TREE_TYPE (arg)) && !vr.undefined_p ())
>>         {
>> -         if (TREE_CODE (arg) == SSA_NAME)
>> -           ipa_set_jfunc_bits (jfunc, 0,
>> -                               widest_int::from (get_nonzero_bits (arg),
>> -                                                 TYPE_SIGN (TREE_TYPE (arg))));
>> -         else
>> -           ipa_set_jfunc_bits (jfunc, wi::to_widest (arg), 0);
>> +         irange &r = as_a <irange> (vr);
>> +         irange_bitmask bm = r.get_bitmask ();
>> +         signop sign = TYPE_SIGN (TREE_TYPE (arg));
>> +         ipa_set_jfunc_bits (jfunc,
>> +                             widest_int::from (bm.value (), sign),
>> +                             widest_int::from (bm.mask (), sign));
>>         }
>>        else if (POINTER_TYPE_P (TREE_TYPE (arg)))
>>         {
>> --
>> 2.40.1
>>
Martin Jambor July 31, 2023, 4:47 p.m. UTC | #4 
Hello,

On Tue, Jul 18 2023, Aldy Hernandez wrote:
> On 7/17/23 15:14, Aldy Hernandez wrote:
>> Instead of reading the known zero bits in IPA, read the value/mask
>> pair which is available.
>> 
>> There is a slight change of behavior here.  I have removed the check
>> for SSA_NAME, as the ranger can calculate the range and value/mask for
>> INTEGER_CST.  This simplifies the code a bit, since there's no special
>> casing when setting the jfunc bits.  The default range for VR is
>> undefined, so I think it's safe just to check for undefined_p().
>
> Final round of tests revealed a regression for which I've adjusted the 
> testcase.
>
> It turns out g++.dg/ipa/pure-const-3.C fails because IPA can now pick up 
> value/mask from any pass that has an integrated ranger.  The test was 
> previously disabling evrp and CCP, but now VRP[12], jump threading, and 
> DOM can make value/mask adjustments visible to IPA so they must be 
> disabled as well.

So can this be then converted into a new testcase that would test that
we can now derive something we could not in the past?

The patch is OK (but the testcase above is highly desirable).

Thanks for keeping looking at IPA-VR.

Martin


>
> We've run into these scenarios multiple times in the past-- any 
> improvements to the ranger pipeline causes everyone to get smarter, 
> making changes visible earlier in the pipeline.
>
> Aldy
> From e1dfd4d6b3d3bf09d55b6ea3ac732462c7030802 Mon Sep 17 00:00:00 2001
> From: Aldy Hernandez <aldyh@redhat.com>
> Date: Fri, 14 Jul 2023 12:38:16 +0200
> Subject: [PATCH] Read global value/mask in IPA.
>
> Instead of reading the known zero bits in IPA, read the value/mask
> pair which is available.
>
> There is a slight change of behavior here.  I have removed the check
> for SSA_NAME, as the ranger can calculate the range and value/mask for
> INTEGER_CST.  This simplifies the code a bit, since there's no special
> casing when setting the jfunc bits.  The default range for VR is
> undefined, so I think it's safe just to check for undefined_p().
>
> gcc/ChangeLog:
>
> 	* ipa-prop.cc (ipa_compute_jump_functions_for_edge): Read global
> 	value/mask.
>
> gcc/testsuite/ChangeLog:
>
> 	* g++.dg/ipa/pure-const-3.C: Adjust for smarter value/mask being
> 	read by ranger earlier than expected by test.
> ---
>  gcc/ipa-prop.cc                         | 18 ++++++++----------
>  gcc/testsuite/g++.dg/ipa/pure-const-3.C |  2 +-
>  2 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
> index 5d790ff1265..4f6ed7b89bd 100644
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> @@ -2402,8 +2402,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
>  	}
>        else
>  	{
> -	  if (TREE_CODE (arg) == SSA_NAME
> -	      && param_type
> +	  if (param_type
>  	      && Value_Range::supports_type_p (TREE_TYPE (arg))
>  	      && Value_Range::supports_type_p (param_type)
>  	      && irange::supports_p (TREE_TYPE (arg))
> @@ -2422,15 +2421,14 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
>  	    gcc_assert (!jfunc->m_vr);
>  	}
>  
> -      if (INTEGRAL_TYPE_P (TREE_TYPE (arg))
> -	  && (TREE_CODE (arg) == SSA_NAME || TREE_CODE (arg) == INTEGER_CST))
> +      if (INTEGRAL_TYPE_P (TREE_TYPE (arg)) && !vr.undefined_p ())
>  	{
> -	  if (TREE_CODE (arg) == SSA_NAME)
> -	    ipa_set_jfunc_bits (jfunc, 0,
> -				widest_int::from (get_nonzero_bits (arg),
> -						  TYPE_SIGN (TREE_TYPE (arg))));
> -	  else
> -	    ipa_set_jfunc_bits (jfunc, wi::to_widest (arg), 0);
> +	  irange &r = as_a <irange> (vr);
> +	  irange_bitmask bm = r.get_bitmask ();
> +	  signop sign = TYPE_SIGN (TREE_TYPE (arg));
> +	  ipa_set_jfunc_bits (jfunc,
> +			      widest_int::from (bm.value (), sign),
> +			      widest_int::from (bm.mask (), sign));
>  	}
>        else if (POINTER_TYPE_P (TREE_TYPE (arg)))
>  	{
> diff --git a/gcc/testsuite/g++.dg/ipa/pure-const-3.C b/gcc/testsuite/g++.dg/ipa/pure-const-3.C
> index b4a4673e86e..e43cf09af27 100644
> --- a/gcc/testsuite/g++.dg/ipa/pure-const-3.C
> +++ b/gcc/testsuite/g++.dg/ipa/pure-const-3.C
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-O2 -fno-ipa-vrp -fdump-tree-optimized -fno-tree-ccp -fdisable-tree-evrp"  } */
> +/* { dg-options "-O2 -fno-ipa-vrp -fdump-tree-optimized -fno-tree-ccp -fdisable-tree-evrp -fdisable-tree-vrp1 -fdisable-tree-vrp2 -fno-thread-jumps -fno-tree-dominator-opts"  } */
>  int *ptr;
>  static int barvar;
>  static int b(int a);
> -- 
> 2.40.1
Aldy Hernandez Aug. 3, 2023, 6:21 p.m. UTC | #5 
On 7/31/23 18:47, Martin Jambor wrote:
> Hello,
> 
> On Tue, Jul 18 2023, Aldy Hernandez wrote:
>> On 7/17/23 15:14, Aldy Hernandez wrote:
>>> Instead of reading the known zero bits in IPA, read the value/mask
>>> pair which is available.
>>>
>>> There is a slight change of behavior here.  I have removed the check
>>> for SSA_NAME, as the ranger can calculate the range and value/mask for
>>> INTEGER_CST.  This simplifies the code a bit, since there's no special
>>> casing when setting the jfunc bits.  The default range for VR is
>>> undefined, so I think it's safe just to check for undefined_p().
>>
>> Final round of tests revealed a regression for which I've adjusted the
>> testcase.
>>
>> It turns out g++.dg/ipa/pure-const-3.C fails because IPA can now pick up
>> value/mask from any pass that has an integrated ranger.  The test was
>> previously disabling evrp and CCP, but now VRP[12], jump threading, and
>> DOM can make value/mask adjustments visible to IPA so they must be
>> disabled as well.
> 
> So can this be then converted into a new testcase that would test that
> we can now derive something we could not in the past?

Good idea.

This is what I'm testing.  I'm basically testing for "Propagated bits" 
in the ipa-cp pass which is where we are able to propagate more things.

I will commit if it succeeds.

Thanks.
Aldy
diff mbox series

Patch

diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
index 5d790ff1265..4f6ed7b89bd 100644
--- a/gcc/ipa-prop.cc
+++ b/gcc/ipa-prop.cc
@@ -2402,8 +2402,7 @@  ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 	}
       else
 	{
-	  if (TREE_CODE (arg) == SSA_NAME
-	      && param_type
+	  if (param_type
 	      && Value_Range::supports_type_p (TREE_TYPE (arg))
 	      && Value_Range::supports_type_p (param_type)
 	      && irange::supports_p (TREE_TYPE (arg))
@@ -2422,15 +2421,14 @@  ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 	    gcc_assert (!jfunc->m_vr);
 	}
 
-      if (INTEGRAL_TYPE_P (TREE_TYPE (arg))
-	  && (TREE_CODE (arg) == SSA_NAME || TREE_CODE (arg) == INTEGER_CST))
+      if (INTEGRAL_TYPE_P (TREE_TYPE (arg)) && !vr.undefined_p ())
 	{
-	  if (TREE_CODE (arg) == SSA_NAME)
-	    ipa_set_jfunc_bits (jfunc, 0,
-				widest_int::from (get_nonzero_bits (arg),
-						  TYPE_SIGN (TREE_TYPE (arg))));
-	  else
-	    ipa_set_jfunc_bits (jfunc, wi::to_widest (arg), 0);
+	  irange &r = as_a <irange> (vr);
+	  irange_bitmask bm = r.get_bitmask ();
+	  signop sign = TYPE_SIGN (TREE_TYPE (arg));
+	  ipa_set_jfunc_bits (jfunc,
+			      widest_int::from (bm.value (), sign),
+			      widest_int::from (bm.mask (), sign));
 	}
       else if (POINTER_TYPE_P (TREE_TYPE (arg)))
 	{