[llvm-commits] [llvm] r99883 - in /llvm/trunk: include/llvm/CodeGen/LiveInterval.h include/llvm/Support/Allocator.h lib/CodeGen/LiveInterval.cpp lib/CodeGen/LiveIntervalAnalysis.cpp lib/Support/Allocator.cpp

[Török Edwin]

On 03/30/2010 07:32 PM, Jakob Stoklund Olesen wrote:
> On Mar 30, 2010, at 4:17 AM, Torok Edwin wrote:
> 
>> Author: edwin
>> Date: Tue Mar 30 06:17:48 2010
>> New Revision: 99883
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=99883&view=rev
>> Log:
>> Reapply r99881 with some fixes: only call destructor in releaseMemory!
>>
>> Modified:
>>    llvm/trunk/include/llvm/CodeGen/LiveInterval.h
>>    llvm/trunk/include/llvm/Support/Allocator.h
>>    llvm/trunk/lib/CodeGen/LiveInterval.cpp
>>    llvm/trunk/lib/CodeGen/LiveIntervalAnalysis.cpp
>>    llvm/trunk/lib/Support/Allocator.cpp
> 
> Hi Török,
> 
> It is OK to defer destruction of VNInfos, and I think it is a good idea with a BumpPtrAllocator that can destroy all elements, but the current implementation seems unsafe. It only works correctly if everything allocated is of the same type and size.

Hi,

I think it is safe in the VNInfo case, but a more general solution is
better of course.

> 
> Given the operator new() at the bottom of Allocator.h, that seems like a risky assertion.

That wasn't there on the 2.7 branch.

> 
> A template class that is only capable of allocating one type would be safer.

Sounds good, the template could automatically call the destructor when
Reset() is called, without the need for the special Reset() method.

I won't have time to work on this for the next days, so if you want to
make these changes please do.

Best regards,
--Edwin