On Fri, Sep 1, 2017 at 6:11 PM, Jeff Law <l...@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
