On 09/02/2017 11:43 AM, Aldy Hernandez wrote:
> 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?
Yes. Thanks for double-checking on the embedded vs space efficient stuff.
jeff
