Re: [PATCH] Avoid some C++ local statics with constructors

[Richard Biener <rguenther at suse dot de>]

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  <jakub@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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)