Re: backwards threader cleanups

[Aldy Hernandez <aldyh at redhat dot com>]

On Fri, Sep 1, 2017 at 6:11 PM, Jeff Law <law@redhat.com> wrote:
> On 09/01/2017 02:18 PM, Aldy Hernandez wrote:
>> Hi.
>>
>> Attached are misc cleanups to tree-ssa-threadbackwards.c and friends.
>> The main gist of the patch is making the path vectors live in the
>> heap, not GC.  But I also cleaned up some comments to reflect reality,
>> and renamed VAR_BB which could use a more meaningful name.  Finally, I
>> abstracted some common code to
>> register_jump_thread_path_if_profitable() in preparation for some
>> upcoming work by me :).
>>
>> Tested on x86-64 Linux.
> It looks like you dropped a level of indirection for path in
> profitable_jump_thread_path and perhaps others that push blocks onto the
> path?   Does your change from having the vectors in the GC space to the
> heap also change them from embeddable vectors to a space efficient
> vector?  It has to for this change to be safe.

Part of my initial motivation was eliminating the double indirection
as well as removing the out-of-line calls to vec_alloc() and
vec_whatever_push().

And yes, the default plain vector<> uses the heap:

template<typename T,
         typename A = va_heap,
         typename L = typename A::default_layout>
struct GTY((user)) vec
{
};

...and va_heap defaults to the space efficient vector:

struct va_heap
{
  typedef vl_ptr default_layout;
...
}

>
> See the discussion in vec.h
>
> I don't recall any inherent reason we use the embedded vector layout.
> It's the default which is probably why it's used here more than anything.

Just a wild guess, but this may be why:

   FIXME - Ideally, they would all be vl_ptr to encourage using regular
           instances for vectors, but the existing GTY machinery is limited
           in that it can only deal with GC objects that are pointers
           themselves.

and:

  /* Use vl_embed as the default layout for GC vectors.  Due to GTY
     limitations, GC vectors must always be pointers, so it is more
     efficient to use a pointer to the vl_embed layout, rather than
     using a pointer to a pointer as would be the case with vl_ptr.  */

>
> Otherwise the change looks good.  I think you just to make sure you're
> not using the embedded layout.  Which I think is just a different type
> when  you declare the vec.

I have made the vectors auto_vec as Trevor suggested.  As auto_vec is
just an inherited plain vec<> with some magic allocation sauce, they
can be passed around interchangeably.

I also changed the hash sets so they live on the stack as opposed to
allocating memory dynamically, at Trevor's request as well.

Bootstraps.  OK pending another round of tests?

Aldy