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
