On Thu, 22 Sep 2016, Jakub Jelinek wrote:
> Hi!
>
> The discovered 5 unnecessary C++ static locals with ctors prompted me to
> look at other cases, which from looking at the optimized or non-optimized
> code are just terrible.
> We don't need to initialize static vectors with vNULL, because that implies
> runtime initialization, static vars are zero initialized, which is what we
> want for the vectors.
> In ipa-cp, I understand the vr.min and vr.max is set just so that
> uninitialized vars aren't copied around, but initializing static local
> wide_int from tree and then copying over is significantly more expensive
> than just using wi::zero.
> The only questionable change is the sreal.h one, what it did is certainly
> very inefficient and ugly, but what the patch does is a kind of hack to keep
> it as efficient as possible even for -O0, at -O2 I'd say the compiler should
> just inline sreal::normalize and fold it into nothing.
> So, if preferred, we could go with just
> inline static sreal min ()
> {
> return sreal (-SREAL_MAX_SIG, SREAL_MAX_EXP);
> }
> which will be at -O2 as efficient as what the patch does - storing 2
> integers.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
While I agree with the goal to reduce the number of static constructors
esp. the vNULL cases I disagree with. This is just introducing
undefined behavior (uninitialized object use), and in case we end up
making vNULL not all-zeros it's bound to break. So IMHO your patch
is very much premature optimization here.
Maybe we can change vNULL to sth that avoids the constructor, if only
if C++14 is available (thus in stage2+)?
For cases like this I hope we could make GCC optimize away the static
construction, maybe you can reduce it to a simple testcase and file
a bugreport? It will require making static constructors "first class"
in the cgraph so we know at some point the function body initializing
a specific global and some later cgraph phase builds up the global
constructor calling all remaining relevant individual constructor
functions (which can then still be inlined into that fn).
Thanks,
Richard.
> 2016-09-22 Jakub Jelinek <ja...@redhat.com>
>
> * ipa-cp.c (ipcp_store_vr_results): Avoid static local
> var zero.
> * sreal.h (sreal::min, sreal::max): Avoid static local vars,
> construct values without normalization.
> * tree-ssa-sccvn.c (vn_reference_lookup_3): Don't initialize
> static local lhs_ops to vNULL.
> cp/
> * name-lookup.c (store_bindings, store_class_bindings): Don't
> initialize static local bindings_need_stored to vNULL.
>
> --- gcc/ipa-cp.c.jj 2016-09-21 08:54:15.000000000 +0200
> +++ gcc/ipa-cp.c 2016-09-22 13:49:57.638198975 +0200
> @@ -5204,11 +5204,9 @@ ipcp_store_vr_results (void)
> }
> else
> {
> - static wide_int zero = integer_zero_node;
> vr.known = false;
> vr.type = VR_VARYING;
> - vr.min = zero;
> - vr.max = zero;
> + vr.min = vr.max = wi::zero (INT_TYPE_SIZE);
> }
> ts->m_vr->quick_push (vr);
> }
> --- gcc/sreal.h.jj 2016-01-04 14:55:52.000000000 +0100
> +++ gcc/sreal.h 2016-09-22 14:09:38.882930801 +0200
> @@ -104,14 +104,20 @@ public:
> /* Global minimum sreal can hold. */
> inline static sreal min ()
> {
> - static sreal min = sreal (-SREAL_MAX_SIG, SREAL_MAX_EXP);
> + sreal min;
> + /* This never needs normalization. */
> + min.m_sig = -SREAL_MAX_SIG;
> + min.m_exp = SREAL_MAX_EXP;
> return min;
> }
>
> /* Global minimum sreal can hold. */
> inline static sreal max ()
> {
> - static sreal max = sreal (SREAL_MAX_SIG, SREAL_MAX_EXP);
> + sreal max;
> + /* This never needs normalization. */
> + max.m_sig = SREAL_MAX_SIG;
> + max.m_exp = SREAL_MAX_EXP;
> return max;
> }
>
> --- gcc/tree-ssa-sccvn.c.jj 2016-09-20 15:43:09.000000000 +0200
> +++ gcc/tree-ssa-sccvn.c 2016-09-22 13:40:03.667886908 +0200
> @@ -1786,8 +1786,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree
> gimple *def_stmt = SSA_NAME_DEF_STMT (vuse);
> tree base = ao_ref_base (ref);
> HOST_WIDE_INT offset, maxsize;
> - static vec<vn_reference_op_s>
> - lhs_ops = vNULL;
> + static vec<vn_reference_op_s> lhs_ops;
> ao_ref lhs_ref;
> bool lhs_ref_ok = false;
>
> --- gcc/cp/name-lookup.c.jj 2016-09-14 23:54:25.000000000 +0200
> +++ gcc/cp/name-lookup.c 2016-09-22 13:51:22.459102089 +0200
> @@ -6197,7 +6197,7 @@ store_binding (tree id, vec<cxx_saved_bi
> static void
> store_bindings (tree names, vec<cxx_saved_binding, va_gc> **old_bindings)
> {
> - static vec<tree> bindings_need_stored = vNULL;
> + static vec<tree> bindings_need_stored;
> tree t, id;
> size_t i;
>
> @@ -6233,7 +6233,7 @@ static void
> store_class_bindings (vec<cp_class_binding, va_gc> *names,
> vec<cxx_saved_binding, va_gc> **old_bindings)
> {
> - static vec<tree> bindings_need_stored = vNULL;
> + static vec<tree> bindings_need_stored;
> size_t i;
> cp_class_binding *cb;
>
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
