diff mbox

Avoid some C++ local statics with constructors

Message ID 20160922195558.GG7282@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Sept. 22, 2016, 7:55 p.m. UTC 
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?

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.


	Jakub

Comments

Richard Biener Sept. 23, 2016, 6:55 a.m. UTC | #1 
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 Sept. 23, 2016, 9:20 a.m. UTC | #2 
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
> > 
> > 
> 
>
diff mbox

Patch

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