Re: [PATCH] rewrite stack vectors

[Trevor Saunders <tsaunders at mozilla dot com>]

On Fri, Oct 25, 2013 at 03:30:47PM -0400, Diego Novillo wrote:
> On 2013-10-10 14:07 , tsaunders@mozilla.com wrote:
> >This makes the implementation of stack vectors simpler and easier to use.  This                                                                                                                                                    works by making the size of the on stack storage a template argument, so the
> >size is embedded in the type.  This allows you to implicitly convert a
> >stack_vec<T, N> to a vec<T, va_heap> *, and it will just work.  Because there's
> >no need to support stack vectors in unions we can make them be a more normal
> >c++ class with a constructor and destructor that are nontrivial.
> >
> 
> Thanks.  This looks much simpler, indeed.  The patch is fine to
> commit.  Just a couple of observations/questions:

I don't have commit access, so can someone check it in for me?  I
bootstrapped and got no changes in regression tests two weeks ago, but
haven't checked it since if that helps.

> >@@ -638,7 +638,7 @@ propagate_threaded_block_debug_into (basic_block dest, basic_block src)
> >        i++;
> >      }
> >-  vec<tree, va_stack> fewvars = vNULL;
> >+  stack_vec<tree, alloc_count> fewvars;
> >    pointer_set_t *vars = NULL;
> 
> Hm, what will happen now if alloc_count == 0?  If I'm following the
> logic, this is tied to the presence of the 'vars' local, so it seems
> that we are fine.  Otherwise, the quick_push operation will run into
> trouble.

The first quick_push is ok because if we were going to add more than
alloc_count vars we would have created the pointer set, and the second
is safe because we explicitly check the vectors length is less than
alloc_count.

> >    /* If we're already starting with 3/4 of alloc_count, go for a
> >@@ -646,8 +646,6 @@ propagate_threaded_block_debug_into (basic_block dest, basic_block src)
> >       VEC.  */
> >    if (i * 4 > alloc_count * 3)
> >      vars = pointer_set_create ();
> >-  else if (alloc_count)
> >-    vec_stack_alloc (tree, fewvars, alloc_count);
> >    /* Now go through the initial debug stmts in DEST again, this time
> >       actually inserting in VARS or FEWVARS.  Don't bother checking for
> >diff --git a/gcc/tree-vect-loop-manip.c b/gcc/tree-vect-loop-manip.c
> >index 574446a..4a14607 100644
> >--- a/gcc/tree-vect-loop-manip.c
> >+++ b/gcc/tree-vect-loop-manip.c
> >@@ -107,7 +107,7 @@ typedef struct
> >     with a PHI DEF that would soon become non-dominant, and when we got
> >     to the suitable one, it wouldn't have anything to substitute any
> >     more.  */
> >-static vec<adjust_info, va_stack> adjust_vec;
> >+static vec<adjust_info, va_heap> adjust_vec;
> 
> A file global was declared as a stack vector?  Sigh.

indeed, istr checking and it could be moved to the stack, but involved
more refactering than seemed to make sense here.

Trev

> 
> 
> 
> Diego.