From: Martin Sebor <msebor@gmail.com>
To: Aldy Hernandez <aldyh@redhat.com>,
GCC patches <gcc-patches@gcc.gnu.org>,
Andrew MacLeod <amacleod@redhat.com>
Cc: Martin Sebor <msebor@redhat.com>
Subject: Re: [PATCH 1/5] Common API for accessing global and on-demand ranges.
Date: Mon, 24 May 2021 13:13:18 -0600 [thread overview]
Message-ID: <2076fedc-de00-2678-eb88-0fb28e16870c@gmail.com> (raw)
In-Reply-To: <20210521113954.1148075-1-aldyh@redhat.com>
On 5/21/21 5:39 AM, Aldy Hernandez via Gcc-patches wrote:
> This patch provides a generic API for accessing global ranges. It is
> meant to replace get_range_info() and get_ptr_nonnull() with one
> common interface. It uses the same API as the ranger (class
> range_query), so there will now be one API for accessing local and
> global ranges alike.
>
> Follow-up patches will convert all users of get_range_info and
> get_ptr_nonnull to this API.
>
> For get_range_info, instead of:
>
> if (!POINTER_TYPE_P (TREE_TYPE (name)) && SSA_NAME_RANGE_INFO (name))
> get_range_info (name, vr);
>
> You can now do:
>
> RANGE_QUERY (cfun)->range_of_expr (vr, name, [stmt]);
>
> ...as well as any other of the range_query methods (range_on_edge,
> range_of_stmt, value_of_expr, value_on_edge, value_on_stmt, etc).
>
> As per the API, range_of_expr will work on constants, SSA names, and
> anything we support in irange::supports_type_p().
>
> For pointers, the interface is the same, so instead of:
>
> else if (POINTER_TYPE_P (TREE_TYPE (name)) && SSA_NAME_PTR_INFO (name))
> {
> if (get_ptr_nonnull (name))
> stuff();
> }
>
> One can do:
>
> RANGE_QUERY (cfun)->range_of_expr (vr, name, [stmt]);
> if (vr.nonzero_p ())
> stuff ();
>
> Along with this interface, we are providing a mechanism by which a
> pass can use an on-demand ranger transparently, without having to
> change its code. Of course, this assumes all get_range_info() and
> get_ptr_nonnull() users have been converted to the new API, which
> follow-up patches will do.
>
> If a pass would prefer to use an on-demand ranger with finer grained
> and context aware ranges, all it would have to do is call
> enable_ranger() at the beginning of the pass, and disable_ranger() at
> the end of the pass.
>
> Note, that to use context aware ranges, any user of range_of_expr()
> would need to pass additional context. For example, the optional
> gimple statement (or perhaps use range_on_edge or range_of_stmt).
>
> The observant reader will note that RANGE_QUERY is tied to cfun, which
> may not be available in certain contexts, such as at RTL time,
> gimple-fold, or some other places where we may or may not have cfun
> set.
>
> For cases where we are sure there is no cfun, you can use
> GLOBAL_RANGE_QUERY instead of RANGE_QUERY(cfun). The API is the same.
>
> For cases where a function may be called with or without a function,
> you could use the following idiom:
>
> range_query *query = cfun ? RANGE_QUERY (cfun) : GLOBAL_RANGE_QUERY;
>
> query->range_of_expr (range, expr, [stmt]);
>
> By default RANGE_QUERY uses GLOBAL_RANGE_QUERY, unless the user has
> enabled an on-demand ranger with enable_ranger(), in which case it
> will use the currently active ranger. That is, until disable_ranger()
> is called, at which point, we revert back to GLOBAL_RANGE_QUERY.
>
> We think this provides a generic way of accessing ranges, both
> globally and locally, without having to keep track of types,
> SSA_NAME_RANGE_INFO, and SSA_NAME_PTR_INFO. We also hope this can be
> used to transition passes from global to on-demand ranges when
> appropriate.
Thanks for the heads up! This sounds great (and thanks for all
the work converting the existing uses). Just a few comments on
the changes.
>
> Tested on x86-64 Linux.
>
> OK?
>
> gcc/ChangeLog:
>
> * function.c (allocate_struct_function): Set cfun->x_range_query.
> * function.h (struct function): Declare x_range_query.
> (RANGE_QUERY): New.
> (get_global_range_query): New.
> (GLOBAL_RANGE_QUERY): New.
> * gimple-range-cache.cc (ssa_global_cache::ssa_global_cache):
> Remove call to safe_grow_cleared.
> * gimple-range.cc (get_range_global): New.
> (gimple_range_global): Move from gimple-range.h.
> (get_global_range_query): New.
> (global_range_query::range_of_expr): New.
> (enable_ranger): New.
> (disable_ranger): New.
> * gimple-range.h (gimple_range_global): Move to gimple-range.cc.
> (class global_range_query): New.
> (enable_ranger): New.
> (disable_ranger): New.
> * gimple-ssa-evrp.c (evrp_folder::~evrp_folder): Rename
> dump_all_value_ranges to dump.
> * tree-vrp.c (vrp_prop::finalize): Same.
> * value-query.cc (range_query::dump): New.
> * value-query.h (range_query::dump): New.
> * vr-values.c (vr_values::dump_all_value_ranges): Rename to...
> (vr_values::dump): ...this.
> * vr-values.h (class vr_values): Rename dump_all_value_ranges to
> dump and make virtual.
> ---
> gcc/function.c | 4 ++
> gcc/function.h | 16 +++++
> gcc/gimple-range-cache.cc | 1 -
> gcc/gimple-range.cc | 130 ++++++++++++++++++++++++++++++++++++++
> gcc/gimple-range.h | 60 +++++-------------
> gcc/gimple-ssa-evrp.c | 2 +-
> gcc/tree-vrp.c | 2 +-
> gcc/value-query.cc | 5 ++
> gcc/value-query.h | 1 +
> gcc/vr-values.c | 2 +-
> gcc/vr-values.h | 2 +-
> 11 files changed, 175 insertions(+), 50 deletions(-)
>
> diff --git a/gcc/function.c b/gcc/function.c
> index fc7b147b5f1..67576950983 100644
> --- a/gcc/function.c
> +++ b/gcc/function.c
> @@ -82,6 +82,8 @@ along with GCC; see the file COPYING3. If not see
> #include "gimple.h"
> #include "options.h"
> #include "function-abi.h"
> +#include "value-range.h"
> +#include "gimple-range.h"
>
> /* So we can assign to cfun in this file. */
> #undef cfun
> @@ -4856,6 +4858,8 @@ allocate_struct_function (tree fndecl, bool abstract_p)
> binding annotations among them. */
> cfun->debug_nonbind_markers = lang_hooks.emits_begin_stmt
> && MAY_HAVE_DEBUG_MARKER_STMTS;
> +
> + cfun->x_range_query = &global_ranges;
> }
>
> /* This is like allocate_struct_function, but pushes a new cfun for FNDECL
> diff --git a/gcc/function.h b/gcc/function.h
> index 66cfa973808..41b446bfe7c 100644
> --- a/gcc/function.h
> +++ b/gcc/function.h
> @@ -157,6 +157,7 @@ struct GTY(()) rtl_eh {
> struct gimple_df;
> struct call_site_record_d;
> struct dw_fde_node;
> +class range_query;
>
> struct GTY(()) varasm_status {
> /* If we're using a per-function constant pool, this is it. */
> @@ -309,6 +310,11 @@ struct GTY(()) function {
> debugging is enabled. */
> struct dw_fde_node *fde;
>
> + /* Range query mechanism for functions. The default is to pick up
> + global ranges. If a pass wants on-demand ranges OTOH, it must
> + call enable/disable_ranger(). */
> + range_query * GTY ((skip)) x_range_query;
> +
Mostly out of curiosity: why the 'x_' prefix? (I'm used to seeing
that in the expansion of the option macros but it seems out of place
here.)
> /* Last statement uid. */
> int last_stmt_uid;
>
> @@ -712,4 +718,14 @@ extern const char *current_function_name (void);
>
> extern void used_types_insert (tree);
>
> +extern range_query *get_global_range_query ();
> +
> +// Returns the currently active range access class. This is meant to be used
> +// with the `class range_query' API. When there is no active range class,
> +// global ranges are used.
> +#define RANGE_QUERY(fun) (fun)->x_range_query
> +
> +// As above, but for accessing global ranges between passes.
> +#define GLOBAL_RANGE_QUERY get_global_range_query ()
Unless there's some reason that escapes me, could these be functions
(possibly defined inline) rather than macros? (They're a lot easier
to work with, such as when I want to trace calls in some way.)
> +
> #endif /* GCC_FUNCTION_H */
> diff --git a/gcc/gimple-range-cache.cc b/gcc/gimple-range-cache.cc
> index 2c922e32913..c645d15f5af 100644
> --- a/gcc/gimple-range-cache.cc
> +++ b/gcc/gimple-range-cache.cc
> @@ -384,7 +384,6 @@ block_range_cache::dump (FILE *f, basic_block bb, bool print_varying)
> ssa_global_cache::ssa_global_cache ()
> {
> m_tab.create (0);
> - m_tab.safe_grow_cleared (num_ssa_names);
> m_irange_allocator = new irange_allocator;
> }
>
> diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
> index 710bc7f9632..4e1aeee8ed0 100644
> --- a/gcc/gimple-range.cc
> +++ b/gcc/gimple-range.cc
> @@ -1402,3 +1402,133 @@ trace_ranger::range_of_expr (irange &r, tree name, gimple *s)
>
> return trailer (idx, "range_of_expr", res, name, r);
> }
> +
> +// Return the legacy global range for NAME if it has one, otherwise
> +// return VARYING.
I realize this is just being moved from elsewhere but I would suggest
to take this opportunity and also document the effects on the range
argument.
> +
> +static void
> +get_range_global (irange &r, tree name)
> +{
> + tree type = TREE_TYPE (name);
> +
> + if (SSA_NAME_IS_DEFAULT_DEF (name))
> + {
> + tree sym = SSA_NAME_VAR (name);
> + // Adapted from vr_values::get_lattice_entry().
> + // Use a range from an SSA_NAME's available range.
> + if (TREE_CODE (sym) == PARM_DECL)
> + {
> + // Try to use the "nonnull" attribute to create ~[0, 0]
> + // anti-ranges for pointers. Note that this is only valid with
> + // default definitions of PARM_DECLs.
> + if (POINTER_TYPE_P (type)
> + && ((cfun && nonnull_arg_p (sym)) || get_ptr_nonnull (name)))
> + r.set_nonzero (type);
> + else if (INTEGRAL_TYPE_P (type))
> + {
> + get_range_info (name, r);
> + if (r.undefined_p ())
> + r.set_varying (type);
> + }
> + else
> + r.set_varying (type);
> + }
> + // If this is a local automatic with no definition, use undefined.
> + else if (TREE_CODE (sym) != RESULT_DECL)
> + r.set_undefined ();
> + else
> + r.set_varying (type);
> + }
> + else if (!POINTER_TYPE_P (type) && SSA_NAME_RANGE_INFO (name))
> + {
> + get_range_info (name, r);
> + if (r.undefined_p ())
> + r.set_varying (type);
> + }
> + else if (POINTER_TYPE_P (type) && SSA_NAME_PTR_INFO (name))
> + {
> + if (get_ptr_nonnull (name))
> + r.set_nonzero (type);
> + else
> + r.set_varying (type);
> + }
> + else
> + r.set_varying (type);
> +}
> +
> +// ?? Like above, but only for default definitions of NAME.
I couldn't help but notice another variation on the dubious ???
yet wide-spread convention:
https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570442.html
> This is
> +// so VRP passes using ranger do not start with known ranges,
> +// otherwise we'd eliminate builtin_unreachables too early because of
> +// inlining.
> +//
> +// Without this restriction, the test in g++.dg/tree-ssa/pr61034.C has
> +// all of its unreachable calls removed too early. We should
> +// investigate whether we should just adjust the test above.
> +
> +value_range
> +gimple_range_global (tree name)
> +{
> + gcc_checking_assert (gimple_range_ssa_p (name));
> + tree type = TREE_TYPE (name);
> +
> + if (SSA_NAME_IS_DEFAULT_DEF (name))
> + {
> + value_range vr;
> + get_range_global (vr, name);
> + return vr;
> + }
> + return value_range (type);
> +}
> +
> +// ----------------------------------------------
> +// global_range_query implementation.
> +
> +global_range_query global_ranges;
> +
> +range_query *
> +get_global_range_query ()
> +{
> + if (cfun)
> + return RANGE_QUERY (cfun);
> +
> + return &global_ranges;
> +}
> +
> +bool
> +global_range_query::range_of_expr (irange &r, tree expr, gimple *)
> +{
> + tree type = TREE_TYPE (expr);
> +
> + if (!irange::supports_type_p (type) || !gimple_range_ssa_p (expr))
> + return get_tree_range (r, expr);
> +
> + get_range_global (r, expr);
> +
> + return true;
> +}
> +
> +void
> +enable_ranger ()
> +{
> + gimple_ranger *r;
> +
> + if (param_evrp_mode & EVRP_MODE_TRACE)
> + r = new trace_ranger;
> + else
> + r = new gimple_ranger;
> +
> + RANGE_QUERY (cfun) = r;
> +}
> +
> +void
> +disable_ranger (bool export_ranges)
> +{
> + gimple_ranger *r = (gimple_ranger *) RANGE_QUERY (cfun);
> +
> + if (export_ranges)
> + r->export_global_ranges ();
> +
> + delete r;
The above is a downcast from range_query*, so if the instance is
something else the code won't work correctly and might crash,
right? Or it might call the wrong function/dtor. Should there
be a check of some sort to make sure that doesn't happen?
Martin
> +
> + RANGE_QUERY (cfun) = &global_ranges;
> +}
> diff --git a/gcc/gimple-range.h b/gcc/gimple-range.h
> index f33156181bf..1ba073820a6 100644
> --- a/gcc/gimple-range.h
> +++ b/gcc/gimple-range.h
> @@ -53,7 +53,7 @@ public:
> virtual void range_on_entry (irange &r, basic_block bb, tree name);
> virtual void range_on_exit (irange &r, basic_block bb, tree name);
> void export_global_ranges ();
> - void dump (FILE *f);
> + virtual void dump (FILE *f) OVERRIDE;
> protected:
> bool calc_stmt (irange &r, gimple *s, tree name = NULL_TREE);
> bool range_of_range_op (irange &r, gimple *s);
> @@ -130,50 +130,6 @@ range_compatible_p (tree type1, tree type2)
> && TYPE_SIGN (type1) == TYPE_SIGN (type2));
> }
>
> -// Return the legacy GCC global range for NAME if it has one, otherwise
> -// return VARYING.
> -
> -static inline value_range
> -gimple_range_global (tree name)
> -{
> - gcc_checking_assert (gimple_range_ssa_p (name));
> - tree type = TREE_TYPE (name);
> -
> - if (SSA_NAME_IS_DEFAULT_DEF (name))
> - {
> - tree sym = SSA_NAME_VAR (name);
> - // Adapted from vr_values::get_lattice_entry().
> - // Use a range from an SSA_NAME's available range.
> - if (TREE_CODE (sym) == PARM_DECL)
> - {
> - // Try to use the "nonnull" attribute to create ~[0, 0]
> - // anti-ranges for pointers. Note that this is only valid with
> - // default definitions of PARM_DECLs.
> - if (POINTER_TYPE_P (type)
> - && (nonnull_arg_p (sym) || get_ptr_nonnull (name)))
> - {
> - value_range r;
> - r.set_nonzero (type);
> - return r;
> - }
> - else if (INTEGRAL_TYPE_P (type))
> - {
> - value_range r;
> - get_range_info (name, r);
> - if (r.undefined_p ())
> - r.set_varying (type);
> - return r;
> - }
> - }
> - // If this is a local automatic with no definition, use undefined.
> - else if (TREE_CODE (sym) != RESULT_DECL)
> - return value_range ();
> - }
> - // Otherwise return range for the type.
> - return value_range (type);
> -}
> -
> -
> // This class overloads the ranger routines to provide tracing facilties
> // Entry and exit values to each of the APIs is placed in the dumpfile.
>
> @@ -202,4 +158,18 @@ private:
> // Temporary external interface to share with vr_values.
> bool range_of_builtin_call (range_query &query, irange &r, gcall *call);
>
> +// Global ranges for SSA names using SSA_NAME_RANGE_INFO.
> +
> +class global_range_query : public range_query
> +{
> +public:
> + bool range_of_expr (irange &r, tree expr, gimple * = NULL) OVERRIDE;
> +};
> +
> +extern global_range_query global_ranges;
> +extern value_range gimple_range_global (tree name);
> +
> +extern void enable_ranger ();
> +extern void disable_ranger (bool export_ranges = true);
> +
> #endif // GCC_GIMPLE_RANGE_STMT_H
> diff --git a/gcc/gimple-ssa-evrp.c b/gcc/gimple-ssa-evrp.c
> index 5f566ae0958..829fdcdaef2 100644
> --- a/gcc/gimple-ssa-evrp.c
> +++ b/gcc/gimple-ssa-evrp.c
> @@ -60,7 +60,7 @@ public:
> if (dump_file)
> {
> fprintf (dump_file, "\nValue ranges after Early VRP:\n\n");
> - m_range_analyzer.dump_all_value_ranges (dump_file);
> + m_range_analyzer.dump (dump_file);
> fprintf (dump_file, "\n");
> }
> }
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index 12e6e6f3e22..b0f1c47f05c 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -4044,7 +4044,7 @@ vrp_prop::finalize ()
> if (dump_file)
> {
> fprintf (dump_file, "\nValue ranges after VRP:\n\n");
> - m_vr_values->dump_all_value_ranges (dump_file);
> + m_vr_values->dump (dump_file);
> fprintf (dump_file, "\n");
> }
>
> diff --git a/gcc/value-query.cc b/gcc/value-query.cc
> index 4bb0897c446..509d2d33cc5 100644
> --- a/gcc/value-query.cc
> +++ b/gcc/value-query.cc
> @@ -135,6 +135,11 @@ range_query::value_of_stmt (gimple *stmt, tree name)
>
> }
>
> +void
> +range_query::dump (FILE *)
> +{
> +}
> +
> // valuation_query support routines for value_range_equiv's.
>
> class equiv_allocator : public object_allocator<value_range_equiv>
> diff --git a/gcc/value-query.h b/gcc/value-query.h
> index e2cbc6852b0..5eff9317ed5 100644
> --- a/gcc/value-query.h
> +++ b/gcc/value-query.h
> @@ -95,6 +95,7 @@ public:
> // rewrite all uses of it to the above API.
> virtual const class value_range_equiv *get_value_range (const_tree,
> gimple * = NULL);
> + virtual void dump (FILE *);
>
> protected:
> class value_range_equiv *allocate_value_range_equiv ();
> diff --git a/gcc/vr-values.c b/gcc/vr-values.c
> index b1bf53af9e0..a89f0b646ae 100644
> --- a/gcc/vr-values.c
> +++ b/gcc/vr-values.c
> @@ -1819,7 +1819,7 @@ vr_values::adjust_range_with_scev (value_range_equiv *vr, class loop *loop,
> /* Dump value ranges of all SSA_NAMEs to FILE. */
>
> void
> -vr_values::dump_all_value_ranges (FILE *file)
> +vr_values::dump (FILE *file)
> {
> size_t i;
>
> diff --git a/gcc/vr-values.h b/gcc/vr-values.h
> index 8c1b2e0a292..81b9131f7f1 100644
> --- a/gcc/vr-values.h
> +++ b/gcc/vr-values.h
> @@ -116,7 +116,7 @@ class vr_values : public range_query
> tree op_with_constant_singleton_value_range (tree);
> void adjust_range_with_scev (value_range_equiv *, class loop *,
> gimple *, tree);
> - void dump_all_value_ranges (FILE *);
> + virtual void dump (FILE *) OVERRIDE;
>
> void extract_range_for_var_from_comparison_expr (tree, enum tree_code,
> tree, tree,
>
next prev parent reply other threads:[~2021-05-24 19:13 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-21 11:39 Aldy Hernandez
2021-05-21 11:39 ` [PATCH 2/5] Convert Walloca pass to RANGE_QUERY(cfun) Aldy Hernandez
2021-05-25 8:43 ` Richard Biener
2021-05-25 16:17 ` Aldy Hernandez
2021-05-25 16:28 ` Jeff Law
2021-05-25 17:28 ` Martin Sebor
2021-05-27 7:38 ` Christophe Lyon
2021-05-27 8:34 ` Aldy Hernandez
2021-05-21 11:39 ` [PATCH 3/5] Convert evrp " Aldy Hernandez
2021-05-25 16:18 ` Aldy Hernandez
2021-05-27 2:34 ` H.J. Lu
2021-05-21 11:39 ` [PATCH 4/5] Convert remaining passes to RANGE_QUERY Aldy Hernandez
2021-05-24 19:34 ` Martin Sebor
2021-05-25 8:47 ` Richard Biener
2021-05-25 9:15 ` Aldy Hernandez
2021-05-25 11:26 ` Aldy Hernandez
2021-05-25 8:48 ` Aldy Hernandez
2021-05-25 16:19 ` Aldy Hernandez
2021-05-21 11:39 ` [PATCH 5/5] Cleanup get_range_info Aldy Hernandez
2021-05-25 16:20 ` Aldy Hernandez
2021-05-24 16:09 ` [PATCH 1/5] Common API for accessing global and on-demand ranges Aldy Hernandez
2021-05-25 8:57 ` Richard Biener
2021-05-25 9:36 ` Aldy Hernandez
2021-05-25 9:46 ` Richard Biener
2021-05-25 10:53 ` Aldy Hernandez
2021-05-25 11:06 ` Richard Biener
2021-05-25 16:16 ` Aldy Hernandez
2021-05-26 13:40 ` Andrew MacLeod
2021-05-29 4:35 ` Jeff Law
2021-05-24 19:13 ` Martin Sebor [this message]
2021-05-25 7:06 ` Aldy Hernandez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2076fedc-de00-2678-eb88-0fb28e16870c@gmail.com \
--to=msebor@gmail.com \
--cc=aldyh@redhat.com \
--cc=amacleod@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=msebor@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).