Re: [COMMITTED] Remove deprecated range_fold_{unary, binary}_expr uses from ipa-*.

[Aldy Hernandez <aldyh@redhat.com>]

On 5/5/23 17:10, Martin Jambor wrote:
> Hello,
> 
> On Wed, Apr 26 2023, Aldy Hernandez via Gcc-patches wrote:
>> gcc/ChangeLog:
>>
>> 	* ipa-cp.cc (ipa_vr_operation_and_type_effects): Convert to ranger API.
>> 	(ipa_value_range_from_jfunc): Same.
>> 	(propagate_vr_across_jump_function): Same.
>> 	* ipa-fnsummary.cc (evaluate_conditions_for_known_args): Same.
>> 	* ipa-prop.cc (ipa_compute_jump_functions_for_edge): Same.
>> 	* vr-values.cc (bounds_of_var_in_loop): Same.
> 
> thanks for taking care of the value range uses in IPA.
> 
>> ---
>>   gcc/ipa-cp.cc        | 28 +++++++++++++++++++++------
>>   gcc/ipa-fnsummary.cc | 45 ++++++++++++++++++++++++++++----------------
>>   gcc/ipa-prop.cc      |  5 ++---
>>   gcc/vr-values.cc     |  6 ++++--
>>   4 files changed, 57 insertions(+), 27 deletions(-)
>>
>> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
>> index 65c49558b58..6788883c40b 100644
>> --- a/gcc/ipa-cp.cc
>> +++ b/gcc/ipa-cp.cc
>> @@ -128,6 +128,7 @@ along with GCC; see the file COPYING3.  If not see
>>   #include "attribs.h"
>>   #include "dbgcnt.h"
>>   #include "symtab-clones.h"
>> +#include "gimple-range.h"
>>   
>>   template <typename valtype> class ipcp_value;
>>   
>> @@ -1900,10 +1901,15 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr,
>>   				   enum tree_code operation,
>>   				   tree dst_type, tree src_type)
>>   {
>> -  range_fold_unary_expr (dst_vr, operation, dst_type, src_vr, src_type);
>> -  if (dst_vr->varying_p () || dst_vr->undefined_p ())
>> +  if (!irange::supports_p (dst_type) || !irange::supports_p (src_type))
>>       return false;
>> -  return true;
>> +
>> +  range_op_handler handler (operation, dst_type);
> 
> Would it be possible to document the range_op_handler class somewhat?

Sorry for the late response, but you're totally right.  We're in dire 
need of documentation here.  I had planned to work on comments and 
actual documentation much later this cycle, but I may need to bump that 
up in priority.

> 
>> +  return (handler
>> +	  && handler.fold_range (*dst_vr, dst_type,
>> +				 *src_vr, value_range (dst_type))
>> +	  && !dst_vr->varying_p ()
>> +	  && !dst_vr->undefined_p ());
> 
> It looks important but the class is not documented at all.  Although the
> use of fold_range is probably hopefully mostly clear from its uses in
> this patch, the meaning of the return value of this method and what
> other methods do is less obvious.
> 
> For example, I am curious why (not in this patch, but in the code as it
> is now in the repo), uses of fold_range seem to be always preceeded with
> a check for supports_type_p, even though the type is then also fed into
> fold_range itself.  Does the return value of fold_range mean something
> slightly different from "could not deduce anything?"

Returning false from fold_range() is a shortcut for I don't know 
anything, which will be treated as VARYING upstream.

The other methods also need documentation.  The most important ones are 
documented in range-op.h:

// This class is implemented for each kind of operator supported by
// the range generator.  It serves various purposes.

particularly op1_range, and op2_range which can be confusing.  But yes, 
we need to revisit this, as those comments are pretty out of date.

Aldy