[llvm-commits] [cfe-commits] [PATCH][Review Request] EarlyCSE stack overflow - bugzilla 11794

[Lenny Maiorani]

On Jan 23, 2012, at 4:03 PM, Jakob Stoklund Olesen wrote:

> 
> On Jan 23, 2012, at 2:30 PM, Lenny Maiorani wrote:
> 
>> Thank you for the information about MachineCSE and noticing the hash tables which are being generated at the top of EarlyCSE::processNode(). My latest implementation contains roughly the same implementation as MachineCSE. MachineCSE generates stack of nodes to process using SmallVector. Since this data structure is so large, I think using SmallVector is not correct.
> 
> SmallVector is almost always a better choice than std::vector, which is essentially equivalent to SmallVector<T, 0>. std::vector only makes sense if you need to create many vector instances that will probably overflow the small size.
> 
>> Also, I implemented the solution using vec.resize() and std::copy(), but this doesn't significantly improve the performance of the vector solution. My numbers are not at all surprising to me. Vectors do not do as well as deques with many pushes and pops because deques manage their memory in slabs to prevent needing to do large reallocs and hence large copies on occasion.
> 
> Interesting. A vector only needs to copy its elements once (amortized).
> 
>> Using the DepthFirstIterator (df_iterator) would be nice, but I do not see an easy way to maintain the CurrentGeneration variable which gets modified on a per-tree-depth. This means that there would still need to be some sort of stack kept separately from the iterator. This would be confusing and clutter the code.
> 
> Yes, unfortunately df_iterator doesn't allow you to use its internal stack. That would have been convenient.
> 
>> I looked a bit at the hash tables at the top of EarlyCSE::processNode() and I do not think they are nested. It looks like they are copy-constructed from some hash tables which are class members. These are then modified and provided the order of node traversal does not change, then the data in those member hash tables should be the same.
> 
> Look again. The implementation is in ScopedHashTable.h.
> 
> When processing a node, there must be a ScopedHashTableScope instantiated for every dominator tree level above the node. The nested scopes form a linked list.
> 
> /jakob
> 

Hi Jakob,

I am getting back to this and it looks like I will need to store the 3 ScopedHashTable objects. I am considering doing this via an IntrusiveRefCntPtr since I will need to store them in the local stack along with the generation and node and each scope might have multiple instances, one instance per child node. Is it preferred to have ScopedHashTable directly inherit from RefCountedBase (making all instances of ScopedHashTable have RefCountedBase) or create a new class within EarlyCSE which inherits and use a naked ptr to the ScopedHashTable inside that class?

Thanks,
-Lenny