From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x333.google.com (mail-ot1-x333.google.com [IPv6:2607:f8b0:4864:20::333]) by sourceware.org (Postfix) with ESMTPS id 5331538930DF for ; Mon, 24 May 2021 19:13:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5331538930DF Received: by mail-ot1-x333.google.com with SMTP id 69-20020a9d0a4b0000b02902ed42f141e1so26240709otg.2 for ; Mon, 24 May 2021 12:13:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=qeDjkrpU1zwvPQomwVeOrGS4utK58VI8b4Q9igYd6vE=; b=C94NNuRR2wSloL2sRE3QlR5wk8TSxudlkajjbbuT2tAJ4f5STB7aPawxnX8JAnFy9R 0QQKLjE5RXuj4Lgeo4FbH4Dvcy9a2pqcO2+O+O4M1Sz/F1zqe7SUTkMbq8K2UbW5b/q+ b/CM0WIRud83DkmHYmnHtP8ujZsg6emVkgeMwkGHGG4rfG7HOvWHk89XXrDfi7iPaUWB 2Ko0Bmk0B2fUu3BhI6RzA0D4U13T4tCbHFzdRBagyAzTsTVagg3jIxuD997jVka/KFcZ cVcOlAJDKmhOVtkoYzFCObOpC+2ZqRGbAVnEyYnC5/LLwo+/EoV8pGJ9zhc1GR6XMyQT XTJw== X-Gm-Message-State: AOAM532kKeMVjhcHx8+OvfQeI/sRo//qLRxNtWnWAHEdP/QMxaxLKc5c ui4DB7idLDk5JFlQdpUl6pQ= X-Google-Smtp-Source: ABdhPJzKxJPLk8J2HM23wGT2CRslK1lJvAorXTGx3gZwBHTYx/+IA9a7K1pvAfVRBSpTxPTuPHwKQQ== X-Received: by 2002:a05:6830:16c4:: with SMTP id l4mr20419153otr.93.1621883600555; Mon, 24 May 2021 12:13:20 -0700 (PDT) Received: from [192.168.0.41] (174-16-126-108.hlrn.qwest.net. [174.16.126.108]) by smtp.gmail.com with ESMTPSA id e29sm2825173oiy.53.2021.05.24.12.13.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 24 May 2021 12:13:19 -0700 (PDT) Subject: Re: [PATCH 1/5] Common API for accessing global and on-demand ranges. To: Aldy Hernandez , GCC patches , Andrew MacLeod Cc: Martin Sebor References: <20210521113954.1148075-1-aldyh@redhat.com> From: Martin Sebor Message-ID: <2076fedc-de00-2678-eb88-0fb28e16870c@gmail.com> Date: Mon, 24 May 2021 13:13:18 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <20210521113954.1148075-1-aldyh@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 24 May 2021 19:13:24 -0000 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 > 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, >