[llvm-commits] [PATCH] Make Inits FoldingSetNodes

[David A. Greene]

Jakob Stoklund Olesen <stoklund at 2pi.dk> writes:

> I agree with Chris that FoldingSet is overkill for this, though.
>
>> +  Init(const FoldingSetNodeID &ID) : FastFoldingSetNode(ID) {}
>
> UnsetInit and BitInit should be singletons (or doubletons if that is a word). DenseMap<int64_t,IntInit*> is hard to beat for interning IntInits.

Ok, but this means introducing separate FoldingSets for each other type
of Init that wants to use them.  Seems much more complicated than one
global pool.  I guess I'll do that if that's what people really want.

> For StringInit and CodeInit, I am not sure interning is even
> necessary. You definitely want to have an empty string singleton, but
> other than that, how many duplicate strings are there really?

There will be a lot of duplicates in the future.  This is all in
preparation for the complete AVX implementation.

>> +  static void ReleaseMemory() {
>> +    InitAllocator.Reset();
>> +  }
>
>
> I don't think there is any reason to release memory. This function
> leaks a ton anyway since it doesn't call destructors, and it is only
> called immediately before exiting the process. It is very hard to beat
> _exit(2) for speed of deallocating complex data structures.

Ok, but will Valgrind report leaks?

> ListInits are interesting. We allocate a ton of them in
> ListRecTy::convertValue() which is a little too naive. It should
> recognize the cases where it didn't change anything, and just return
> the input.
>
> I think you could eliminate almost all duplicate ListInits by:
>
> - Teaching ListRecTy::convertValue() to avoid creating gratuitous duplicates.
>
> - Interning empty lists by element type.
>
> - Interning singleton lists by their single element.
>
> That would likely have less overhead than using a FoldingSet. I don't
> think the multi-element lists have enough duplicates that it is worth
> interning them.

This is way beyond the scope of this patch.  And again, there are going
to be more lists in the future and many of them will probably look the
same.

> I was looking at TableGen's memory usage a while ago. I attacked the
> RecTy instances instead of the Inits, but I used the same basic idea
> of interning. TableGen is allocating some 200k ListRecTy instances
> when parsing ARM.td ;-)

Yes, RecTys are a huge problem too.

> I have a patch that interns all the RecTy objects. It reduces TableGen
> heap usage from 24 MB to 16 MB IIRC. That is a lot in relative terms,
> but not in absolute terms, and it didn't improve parse time by much. I
> didn't think it was worth the churn at the time, but since you are
> churning in the same code now, I can send you the patch if you are
> interested.

No need to send it to me.  Just commit it once this stuff is settled.  I
think it will be a win.  If nothing else it will clean up valgrind
output a lot and that's worth quite a bit.

                            -Dave