Re: [PATCH] Read global value/mask in IPA.

[Martin Jambor <mjambor@suse.cz>]

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