Re: Propagate value ranges of return values

[Martin Jambor <mjambor@suse.cz>]

Hi,

thanks for working on this.

On Sun, Nov 19 2023, Jan Hubicka wrote:
> Hi,
> this is updated version which also adds testuiste compensation
> I lost earlier while maintaining the patch in my testing tree.
> There are quite few testcases that use constant return values to hide
> something from optimizer.
>
> Bootstrapped/regtested x86_64-linux.
> gcc/ChangeLog:
>
> 	* cgraph.cc (add_detected_attribute_1): New function.
> 	(cgraph_node::add_detected_attribute): Likewise.
> 	* cgraph.h (cgraph_node::add_detected_attribute): Declare.
> 	* common.opt: Add -Wsuggest-attribute=returns_nonnull.
> 	* doc/invoke.texi: Document new flag.
> 	* gimple-range-fold.cc (fold_using_range::range_of_call):
> 	Use known reutrn value ranges.
> 	* ipa-prop.cc (struct ipa_return_value_summary): New type.
> 	(class ipa_return_value_sum_t): New type.
> 	(ipa_return_value_sum): New summary.
> 	(ipa_record_return_value_range): New function.
> 	(ipa_return_value_range): New function.
> 	* ipa-prop.h (ipa_return_value_range): Declare.
> 	(ipa_record_return_value_range): Declare.
> 	* ipa-pure-const.cc (warn_function_returns_nonnull): New funcion.
> 	* ipa-utils.h (warn_function_returns_nonnull): Declare.
> 	* symbol-summary.h: Fix comment.
> 	* tree-vrp.cc (execute_ranger_vrp): Record return values.
>
> gcc/testsuite/ChangeLog:
>
> 	* g++.dg/ipa/devirt-2.C: Add noipa attribute to prevent ipa-vrp.
> 	* g++.dg/ipa/devirt-7.C: Disable ipa-vrp.
> 	* g++.dg/ipa/ipa-icf-2.C: Disable ipa-vrp.
> 	* g++.dg/ipa/ipa-icf-3.C: Disable ipa-vrp.
> 	* g++.dg/ipa/ivinline-1.C: Disable ipa-vrp.
> 	* g++.dg/ipa/ivinline-3.C: Disable ipa-vrp.
> 	* g++.dg/ipa/ivinline-5.C: Disable ipa-vrp.
> 	* g++.dg/ipa/ivinline-8.C: Disable ipa-vrp.
> 	* g++.dg/ipa/nothrow-1.C: Disable ipa-vrp.
> 	* g++.dg/ipa/pure-const-1.C: Disable ipa-vrp.
> 	* g++.dg/ipa/pure-const-2.C: Disable ipa-vrp.
> 	* g++.dg/lto/inline-crossmodule-1_0.C: Disable ipa-vrp.
> 	* gcc.c-torture/compile/pr106433.c: Add noipa attribute to prevent ipa-vrp.
> 	* gcc.c-torture/execute/frame-address.c: Likewise.
> 	* gcc.dg/ipa/fopt-info-inline-1.c: Disable ipa-vrp.
> 	* gcc.dg/ipa/ipa-icf-25.c: Disable ipa-vrp.
> 	* gcc.dg/ipa/ipa-icf-38.c: Disable ipa-vrp.
> 	* gcc.dg/ipa/pure-const-1.c: Disable ipa-vrp.
> 	* gcc.dg/ipa/remref-0.c: Add noipa attribute to prevent ipa-vrp.
> 	* gcc.dg/tree-prof/time-profiler-1.c: Disable ipa-vrp.
> 	* gcc.dg/tree-prof/time-profiler-2.c: Disable ipa-vrp.
> 	* gcc.dg/tree-ssa/pr110269.c: Disable ipa-vrp.
> 	* gcc.dg/tree-ssa/pr20701.c: Disable ipa-vrp.
> 	* gcc.dg/tree-ssa/vrp05.c: Disable ipa-vrp.
> 	* gcc.dg/tree-ssa/return-value-range-1.c: New test.
>
> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
> index e41e5ad3ae7..71dacf23ce1 100644
> --- a/gcc/cgraph.cc
> +++ b/gcc/cgraph.cc
> @@ -2629,6 +2629,54 @@ cgraph_node::set_malloc_flag (bool malloc_p)
>    return changed;
>  }
>  
> +/* Worker to set malloc flag.  */

I think the comment must be stale, and the name of the function also, it
does not add anything, does it?

> +static void
> +add_detected_attribute_1 (cgraph_node *node, const char *attr, bool *changed)
> +{
> +  if (!lookup_attribute (attr, DECL_ATTRIBUTES (node->decl)))
> +    {
> +      DECL_ATTRIBUTES (node->decl) = tree_cons (get_identifier (attr),
> +					 NULL_TREE, DECL_ATTRIBUTES (node->decl));
> +      *changed = true;
> +    }
> +
> +  ipa_ref *ref;
> +  FOR_EACH_ALIAS (node, ref)
> +    {
> +      cgraph_node *alias = dyn_cast<cgraph_node *> (ref->referring);
> +      if (alias->get_availability () > AVAIL_INTERPOSABLE)
> +	add_detected_attribute_1 (alias, attr, changed);
> +    }
> +
> +  for (cgraph_edge *e = node->callers; e; e = e->next_caller)
> +    if (e->caller->thunk
> +	&& (e->caller->get_availability () > AVAIL_INTERPOSABLE))
> +      add_detected_attribute_1 (e->caller, attr, changed);
> +}
> +
> +/* Set DECL_IS_MALLOC on NODE's decl and on NODE's aliases if any.  */

Likewise.

> +
> +bool
> +cgraph_node::add_detected_attribute (const char *attr)
> +{
> +  bool changed = false;
> +
> +  if (get_availability () > AVAIL_INTERPOSABLE)
> +    add_detected_attribute_1 (this, attr, &changed);
> +  else
> +    {
> +      ipa_ref *ref;
> +
> +      FOR_EACH_ALIAS (this, ref)
> +	{
> +	  cgraph_node *alias = dyn_cast<cgraph_node *> (ref->referring);
> +	  if (alias->get_availability () > AVAIL_INTERPOSABLE)
> +	    add_detected_attribute_1 (alias, attr, &changed);
> +	}
> +    }
> +  return changed;
> +}
> +
>  /* Worker to set noreturng flag.  */
>  static void
>  set_noreturn_flag_1 (cgraph_node *node, bool noreturn_p, bool *changed)

[...]

> diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
> index 6e9530c3d7f..998b7608d78 100644
> --- a/gcc/gimple-range-fold.cc
> +++ b/gcc/gimple-range-fold.cc
> @@ -44,6 +44,11 @@ along with GCC; see the file COPYING3.  If not see
>  #include "value-query.h"
>  #include "gimple-range-op.h"
>  #include "gimple-range.h"
> +#include "cgraph.h"
> +#include "alloc-pool.h"
> +#include "symbol-summary.h"
> +#include "ipa-utils.h"
> +#include "ipa-prop.h"
>  // Construct a fur_source, and set the m_query field.
>  
>  fur_source::fur_source (range_query *q)
> @@ -1013,6 +1018,25 @@ fold_using_range::range_of_call (vrange &r, gcall *call, fur_source &)
>    else
>      r.set_varying (type);
>  
> +  tree callee = gimple_call_fndecl (call);
> +  if (callee
> +      && useless_type_conversion_p (TREE_TYPE (TREE_TYPE (callee)), type))
> +    {
> +      Value_Range val;
> +      if (ipa_return_value_range (val, callee))
> +	{
> +	  r.intersect (val);
> +	  if (dump_file && (dump_flags & TDF_DETAILS))
> +	    {
> +	      fprintf (dump_file, "Using return value range of ");
> +	      print_generic_expr (dump_file, callee, TDF_SLIM);
> +	      fprintf (dump_file, ": ");
> +	      val.dump (dump_file);
> +	      fprintf (dump_file, "\n");
> +	    }
> +	}
> +    }
> +
>    // If there is an LHS, intersect that with what is known.
>    if (lhs)
>      {
> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
> index 7de2b788185..e77bc9c340b 100644
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> @@ -237,6 +237,35 @@ gt_ggc_mx (ipa_vr *&x)
>    return gt_ggc_mx ((ipa_vr *) x);
>  }
>  
> +/* Analysis summery of function call return value.  */
> +struct GTY(()) ipa_return_value_summary
> +{
> +  /* Known value range.
> +     This needs to be wrapped in struccture due to specific way

structure.

> +     we allocate ipa_vr. */
> +  ipa_vr *vr;
> +};
> +
> +/* Function summary for return values.  */
> +class ipa_return_value_sum_t : public function_summary <ipa_return_value_summary *>
> +{
> +public:
> +  ipa_return_value_sum_t (symbol_table *table, bool ggc):
> +    function_summary <ipa_return_value_summary *> (table, ggc) { }
> +
> +  /* Hook that is called by summary when a node is duplicated.  */
> +  void duplicate (cgraph_node *,
> +		  cgraph_node *,
> +		  ipa_return_value_summary *data,
> +		  ipa_return_value_summary *data2) final override
> +  {
> +    *data2=*data;
> +  }
> +};
> +
> +/* Variable hoding the return value summary.  */
> +static GTY(()) function_summary <ipa_return_value_summary *> *ipa_return_value_sum;
> +
>  
>  /* Return true if DECL_FUNCTION_SPECIFIC_OPTIMIZATION of the decl associated
>     with NODE should prevent us from analyzing it for the purposes of IPA-CP.  */
> @@ -5915,5 +5944,49 @@ ipcp_transform_function (struct cgraph_node *node)
>    return modified_mem_access ? TODO_update_ssa_only_virtuals : 0;
>  }
>  
> +/* Record that current function return value range is VAL.  */
> +
> +void
> +ipa_record_return_value_range (Value_Range val)
> +{
> +  cgraph_node *n = cgraph_node::get (current_function_decl);
> +  if (!ipa_return_value_sum)
> +    {
> +      if (!ipa_vr_hash_table)
> +	ipa_vr_hash_table = hash_table<ipa_vr_ggc_hash_traits>::create_ggc (37);
> +      ipa_return_value_sum = new (ggc_alloc_no_dtor <ipa_return_value_sum_t> ())
> +	      ipa_return_value_sum_t (symtab, true);
> +      ipa_return_value_sum->disable_insertion_hook ();
> +    }
> +  ipa_return_value_sum->get_create (n)->vr = ipa_get_value_range (val);
> +  if (dump_file && (dump_flags & TDF_DETAILS))
> +    {
> +      fprintf (dump_file, "Recording return range ");
> +      val.dump (dump_file);
> +      fprintf (dump_file, "\n");
> +    }
> +}
> +
> +/* Return true if value range of DECL is known and if so initialize
> RANGE.  */

Perhaps better: "If value range of the value returned by function DECL
is known, return true and set RANGE to it."  But I guess it does not
matter much.

> +
> +bool
> +ipa_return_value_range (Value_Range &range, tree decl)
> +{
> +  cgraph_node *n = cgraph_node::get (decl);
> +  if (!n || !ipa_return_value_sum)
> +    return false;
> +  enum availability avail;
> +  n = n->ultimate_alias_target (&avail);
> +  if (avail < AVAIL_AVAILABLE)
> +    return false;
> +  if (n->decl != decl && !useless_type_conversion_p (TREE_TYPE (decl), TREE_TYPE (n->decl)))
> +    return false;
> +  ipa_return_value_summary *v = ipa_return_value_sum->get (n);
> +  if (!v)
> +    return false;
> +  v->vr->get_vrange (range);
> +  return true;
> +}
> +
>  
>  #include "gt-ipa-prop.h"
> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> index fcd0e5c638f..5901c805c40 100644
> --- a/gcc/ipa-prop.h
> +++ b/gcc/ipa-prop.h
> @@ -309,7 +309,7 @@ public:
>    void get_vrange (Value_Range &) const;
>    bool equal_p (const vrange &) const;
>    const vrange_storage *storage () const { return m_storage; }
> -  void streamer_read (lto_input_block *, data_in *);
> +  void streamer_read (lto_input_block *, class data_in *);

Is this an intended change?  Or is there some other hunk for
streamer_read missing?


>    void streamer_write (output_block *) const;
>    void dump (FILE *) const;
>  
> @@ -1274,4 +1274,7 @@ ipa_range_set_and_normalize (vrange &r, tree val)
>      r.set (val, val);
>  }
>  
> +bool ipa_return_value_range (Value_Range &range, tree decl);
> +void ipa_record_return_value_range (Value_Range val);
> +
>  #endif /* IPA_PROP_H */

[...]

> diff --git a/gcc/tree-vrp.cc b/gcc/tree-vrp.cc
> index 917fa873714..82001eff20e 100644
> --- a/gcc/tree-vrp.cc
> +++ b/gcc/tree-vrp.cc
> @@ -52,6 +52,12 @@ along with GCC; see the file COPYING3.  If not see
>  #include "gimple-fold.h"
>  #include "tree-dfa.h"
>  #include "tree-ssa-dce.h"
> +#include "alloc-pool.h"
> +#include "cgraph.h"
> +#include "symbol-summary.h"
> +#include "ipa-utils.h"
> +#include "ipa-prop.h"
> +#include "attribs.h"
>  
>  // This class is utilized by VRP and ranger to remove __builtin_unreachable
>  // calls, and reflect any resulting global ranges.
> @@ -1081,6 +1087,51 @@ execute_ranger_vrp (struct function *fun, bool warn_array_bounds_p,
>        array_checker.check ();
>      }
>  
> +
> +  if (Value_Range::supports_type_p (TREE_TYPE
> +				     (TREE_TYPE (current_function_decl)))
> +      && flag_ipa_vrp
> +      && !lookup_attribute ("noipa", DECL_ATTRIBUTES (current_function_decl)))
> +    {
> +      edge e;
> +      edge_iterator ei;
> +      bool found = false;
> +      Value_Range return_range (TREE_TYPE (TREE_TYPE (current_function_decl)));
> +      FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds)
> +	if (greturn *ret = dyn_cast <greturn *> (*gsi_last_bb (e->src)))
> +	  {
> +	    tree retval = gimple_return_retval (ret);
> +	    if (!retval)
> +	      {
> +		return_range.set_varying (TREE_TYPE (TREE_TYPE (current_function_decl)));
> +		found = true;
> +		continue;
> +	      }
> +	    Value_Range r (TREE_TYPE (retval));
> +	    if (ranger->range_of_expr (r, retval, ret)
> +		&& !r.undefined_p ()
> +		&& !r.varying_p ())
> +	      {
> +		if (!found)
> +		  return_range = r;
> +		else
> +		  return_range.union_ (r);
> +	      }
> +	    else
> +	      return_range.set_varying (TREE_TYPE (retval));
> +	    found = true;
> +	  }
> +      if (found && !return_range.varying_p ())
> +	{
> +	  ipa_record_return_value_range (return_range);
> +	  if (POINTER_TYPE_P (TREE_TYPE (TREE_TYPE (current_function_decl)))
> +	      && return_range.nonzero_p ()
> +	      && cgraph_node::get (current_function_decl)
> +			->add_detected_attribute ("returns_nonnull"))
> +	    warn_function_returns_nonnull (current_function_decl);
> +	}
> +    }
> +
>    phi_analysis_finalize ();
>    disable_ranger (fun);
>    scev_finalize ();

Otherwise it looks great.

Thanks,

Martin