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

[Richard Biener]

On Fri, 23 Sep 2016, Richard Biener wrote:

> 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).

Thus in the light of the other patch-set this patch is ok as well.

Thanks,
Richard.

> 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)