Issue 71042: Patch to allow the JITMemoryManager to allocate more blocks of memory. (Closed)
|
Created:
15 years, 7 months ago by Reid Kleckner Modified:
15 years, 5 months ago CC:
unladen-swallow_googlegroups.com Base URL:
http://llvm.org/svn/llvm-project/llvm/trunk/ Visibility:
Public. |
DescriptionThis fixes the FIXMEs for stubs, globals, and code.
For stubs and global data, the memory manager uses the simple BumpAllocator class, which keeps a vector of memory slabs and a pair of pointers that it bumps along to make allocations. For especially large allocations, it allocates a separate slab. This is done so that in the worst case it wastes at most an amount of space equal to the threshold at the end of the block, or a page at the end of the separate slab.
For code, the memory manager adds the ability to request more memory by allocating more code slabs and adding them to the free list. If memory gets fragmented, this could become a problem, since each allocation loops over the entire free list.
When the JITEmitter runs out of memory, it now frees the memory it has been working with so far, asks for twice as much memory, and tries again until it succeeds.
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressed comments, but the code is still buggy. #Patch Set 3 : Added unittests to exercise the code. #Patch Set 4 : Patch to clear out old relocations when reemitting code. #
Total comments: 34
Patch Set 5 : Addressed Jeffrey's comments. Fixed up CheckInvariants. #
Total comments: 45
Patch Set 6 : Fixed style issues. #Patch Set 7 : Updated to use a bump allocator for both global data and function stubs #
Total comments: 41
Patch Set 8 : Addressed review comments. #
Total comments: 6
Patch Set 9 : ... #Patch Set 10 : Modified LLI to make nightly tests pass with the new memory manager. #Patch Set 11 : Merged bump allocators. #Patch Set 12 : ... #
Total comments: 8
Patch Set 13 : Templatized BumpPtrAllocator. #
MessagesTotal messages: 17
http://codereview.appspot.com/71042/diff/1/3 File lib/ExecutionEngine/JIT/JITEmitter.cpp (right): http://codereview.appspot.com/71042/diff/1/3#newcode1061 Line 1061: DOUT << "JIT: Ran out of space for native code. Reattempting.\n"; It's probably worth adding a Statistic to count this since it's likely to be expensive: http://llvm.org/docs/ProgrammersManual.html#Statistic http://codereview.appspot.com/71042/diff/1/3#newcode1062 Line 1062: deallocateMemForFunction(F.getFunction()); JITMemoryManager.h says, "This is never called when the JIT is currently emitting a function." I think it actually has no effect where you're calling it, since it uses the FunctionBlocks map to find the allocation, and that's not filled in until you call endFunctionBody(). http://codereview.appspot.com/71042/diff/1/3#newcode1064 Line 1064: MinSize = (uintptr_t)(2 * (BufferEnd - BufferBegin)); It looks like the calculation of ActualSize above is exact. We might want to just use that when we have to retry. On the other hand, simpler is better, at least until we have a measurement that it's actually causing problems. http://codereview.appspot.com/71042/diff/1/4 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/1/4#newcode269 Line 269: static const unsigned NewBlockSize = 4 << 20; Remember to define space for NewBlockSize at namespace scope: http://publib.boulder.ibm.com/infocenter/comphelp/v8v101/index.jsp?topic=/com... http://codereview.appspot.com/71042/diff/1/4#newcode296 Line 296: // Search for the largest block. Isn't it a free block? I don't think we want the largest allocated block. http://codereview.appspot.com/71042/diff/1/4#newcode308 Line 308: candidateBlock = allocateNewBlock(); It looks like it would be really easy to pass the ActualSize into allocateNewBlock and make the below assertion impossible (unless mmap fails). http://codereview.appspot.com/71042/diff/1/4#newcode309 Line 309: assert(candidateBlock->BlockSize >= ActualSize && This would infinite loop in production, so I think you should make it an if() {print; exit(1)} instead of an assert(). http://codereview.appspot.com/71042/diff/1/4#newcode328 Line 328: FreeRangeHeader *newBlock = (FreeRangeHeader*)B.base(); I think you have to allocate at least that small allocated block at the end of the new memory. Otherwise MemoryRangeHeader::FreeBlock() (at least) will try to read past the end of the block. I don't think you need the small free and second small allocated blocks on anything but the first allocation. http://codereview.appspot.com/71042/diff/1/4#newcode330 Line 330: newBlock->PrevAllocated = FreeMemoryList->Prev->ThisAllocated; You want to set PrevAllocated to true here, or MemoryRangeHeader::getFreeBlockBefore() will read memory outside the block.
Sign in to reply to this message.
I also added some unittests to exercise this code, which pass. I think it all works, but I need to do more testing with Python before I'd say it's done. http://codereview.appspot.com/71042/diff/1/3 File lib/ExecutionEngine/JIT/JITEmitter.cpp (right): http://codereview.appspot.com/71042/diff/1/3#newcode1061 Line 1061: DOUT << "JIT: Ran out of space for native code. Reattempting.\n"; On 2009/06/11 04:56:44, Jeffrey Yasskin wrote: > It's probably worth adding a Statistic to count this since it's likely to be > expensive: http://llvm.org/docs/ProgrammersManual.html#Statistic Done. http://codereview.appspot.com/71042/diff/1/3#newcode1062 Line 1062: deallocateMemForFunction(F.getFunction()); On 2009/06/11 04:56:44, Jeffrey Yasskin wrote: > JITMemoryManager.h says, "This is never called when the JIT is currently > emitting a function." I think it actually has no effect where you're calling it, > since it uses the FunctionBlocks map to find the allocation, and that's not > filled in until you call endFunctionBody(). Right, so what you're saying is that I need to call endFunctionBody before calling this will have any effect. Done. http://codereview.appspot.com/71042/diff/1/3#newcode1064 Line 1064: MinSize = (uintptr_t)(2 * (BufferEnd - BufferBegin)); On 2009/06/11 04:56:44, Jeffrey Yasskin wrote: > It looks like the calculation of ActualSize above is exact. We might want to > just use that when we have to retry. On the other hand, simpler is better, at > least until we have a measurement that it's actually causing problems. When I poked around at how it was computing the size, it seems that not all ISA's support pre-computing the size, so I figured it would be safer this way. http://codereview.appspot.com/71042/diff/1/4 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/1/4#newcode269 Line 269: static const unsigned NewBlockSize = 4 << 20; On 2009/06/11 04:56:44, Jeffrey Yasskin wrote: > Remember to define space for NewBlockSize at namespace scope: > http://publib.boulder.ibm.com/infocenter/comphelp/v8v101/index.jsp?topic=/com... Done, I think. C++ is weird. http://codereview.appspot.com/71042/diff/1/4#newcode296 Line 296: // Search for the largest block. On 2009/06/11 04:56:44, Jeffrey Yasskin wrote: > Isn't it a free block? I don't think we want the largest allocated block. Done, I rewrote that comment and forgot to put it back exactly. http://codereview.appspot.com/71042/diff/1/4#newcode308 Line 308: candidateBlock = allocateNewBlock(); On 2009/06/11 04:56:44, Jeffrey Yasskin wrote: > It looks like it would be really easy to pass the ActualSize into > allocateNewBlock and make the below assertion impossible (unless mmap fails). Done. http://codereview.appspot.com/71042/diff/1/4#newcode309 Line 309: assert(candidateBlock->BlockSize >= ActualSize && On 2009/06/11 04:56:44, Jeffrey Yasskin wrote: > This would infinite loop in production, so I think you should make it an if() > {print; exit(1)} instead of an assert(). Now it should be impossible for the assert to fail, but I'm going to leave it as is. http://codereview.appspot.com/71042/diff/1/4#newcode328 Line 328: FreeRangeHeader *newBlock = (FreeRangeHeader*)B.base(); On 2009/06/11 04:56:44, Jeffrey Yasskin wrote: > I think you have to allocate at least that small allocated block at the end of > the new memory. Otherwise MemoryRangeHeader::FreeBlock() (at least) will try to > read past the end of the block. I don't think you need the small free and second > small allocated blocks on anything but the first allocation. I thought I understood it and I thought it didn't need that, but you're right. Good catch. http://codereview.appspot.com/71042/diff/1/4#newcode330 Line 330: newBlock->PrevAllocated = FreeMemoryList->Prev->ThisAllocated; On 2009/06/11 04:56:44, Jeffrey Yasskin wrote: > You want to set PrevAllocated to true here, or > MemoryRangeHeader::getFreeBlockBefore() will read memory outside the block. Done.
Sign in to reply to this message.
http://codereview.appspot.com/71042/diff/5013/5019 File lib/ExecutionEngine/JIT/JITEmitter.cpp (right): http://codereview.appspot.com/71042/diff/5013/5019#newcode1070 Line 1070: MemMgr->endFunctionBody(F.getFunction(), BufferBegin, CurBufferPtr); I'm not particularly happy having to do all the work of ending the function body just to deallocate it, but I don't see an obviously better way to do it. Be prepared for someone on llvm-commits to suggest the better way. ;) http://codereview.appspot.com/71042/diff/5013/5019#newcode1266 Line 1266: Relocations.clear(); Does your test have a case that fails without this? http://codereview.appspot.com/71042/diff/5013/5020 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (left): http://codereview.appspot.com/71042/diff/5013/5020#oldcode256 Line 256: Usually try to avoid changing whitespace far away from your real changes. http://codereview.appspot.com/71042/diff/5013/5020 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/5013/5020#newcode594 Line 594: bool DefaultJITMemoryManager::CheckInvariants() { I would give this either an out string parameter or a string result type so that you can describe why the invariant check failed. If you were to use a string result type, you'd use empty to mark success. http://codereview.appspot.com/71042/diff/5013/5020#newcode596 Line 596: I != Blocks.end(); ++I) { Chris hates this. Always cache the .end() result in an E variable declared in the for loop setup. http://codereview.appspot.com/71042/diff/5013/5020#newcode598 Line 598: uint8_t *start = (uint8_t*)I->base(); Stylistically, LLVM capitalizes its local variables. (I think they're crazy, but consistency wins. :) http://codereview.appspot.com/71042/diff/5013/5020#newcode601 Line 601: MemoryRangeHeader *lastHdr = NULL; In C++, always try to declare variables where they're initialized. http://codereview.appspot.com/71042/diff/5013/5020#newcode610 Line 610: while (iter != head) { Use a SmallPtrSet to store the set of free blocks to avoid the O(N^3) loops here. http://codereview.appspot.com/71042/diff/5013/5020#newcode624 Line 624: return false; // Bad Next pointer. Why doesn't this always return false when there's more than one block? It seems like blocks are guaranteed not to overlap, so there will always be at least one block for which iter doesn't fall between the start and the end. http://codereview.appspot.com/71042/diff/5013/5020#newcode645 Line 645: hdr = &hdr->getBlockAfter(); How do you know that the block after hdr is valid? Can't it have fallen off the end of the slab? http://codereview.appspot.com/71042/diff/5013/5020#newcode646 Line 646: if (lastHdr && lastHdr->ThisAllocated != hdr->PrevAllocated) { If lastHdr is NULL here, you would have crashed earlier in accessing hdr->ThisAllocated, etc. Do you mean to check hdr? No, because it's the address of a reference. What's going on? http://codereview.appspot.com/71042/diff/5013/5015 File unittests/ExecutionEngine/JIT/JITMemoryManagerTest.cpp (right): http://codereview.appspot.com/71042/diff/5013/5015#newcode27 Line 27: JITMemoryManager *MemMgr = JITMemoryManager::CreateDefaultMemManager(); Use an OwningPtr here: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/OwningPtr.h?v... http://codereview.appspot.com/71042/diff/5013/5015#newcode38 Line 38: TEST(JITMemoryManagerTest, ThreeFunction) { What is this checking that the OneFunction test didn't? Try to name the test according to what it's checking. http://codereview.appspot.com/71042/diff/5013/5015#newcode81 Line 81: // Make three large functions and then try and allocate three small functions. Mention why this is significant. Are you trying to get the small functions to be allocated inside the slabs for the large functions? If so, how do you know they're actually getting allocated there? http://codereview.appspot.com/71042/diff/5013/5015#newcode88 Line 88: uintptr_t bigFuncSize = MemMgr->GetNewBlockSize() - (2 * 1024); Make these const so that readers know they don't change accidentally. http://codereview.appspot.com/71042/diff/5013/5015#newcode91 Line 91: // one We don't really need a comment telling us that the next line is going to say, "F1". We can just look at the next line. http://codereview.appspot.com/71042/diff/5013/5015#newcode135 Line 135: // deallocate two This comment looks like you're trying to deallocate two of the functions, not function #2. The "function #2" interpretation actually doesn't need a comment, since the next line says, deallocateMemForFunction(F2).
Sign in to reply to this message.
Thanks Jeffrey, there was a lot to improve. http://codereview.appspot.com/71042/diff/5013/5019 File lib/ExecutionEngine/JIT/JITEmitter.cpp (right): http://codereview.appspot.com/71042/diff/5013/5019#newcode1070 Line 1070: MemMgr->endFunctionBody(F.getFunction(), BufferBegin, CurBufferPtr); On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > I'm not particularly happy having to do all the work of ending the function body > just to deallocate it, but I don't see an obviously better way to do it. > Be prepared for someone on llvm-commits to suggest the better way. ;) I can think of a couple of ways, but most of them involve refactoring endFunctionBody and deallocateMemForFunction. I could do that, but I was trying to do the simplest thing that worked, not the fastest. http://codereview.appspot.com/71042/diff/5013/5019#newcode1266 Line 1266: Relocations.clear(); On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > Does your test have a case that fails without this? No, I don't have tests that deal with code generation. If we wanted to test this, I'd make a JITEmitterTest that tried to test code generation. I should probably comment that line, though. http://codereview.appspot.com/71042/diff/5013/5020 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (left): http://codereview.appspot.com/71042/diff/5013/5020#oldcode256 Line 256: On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > Usually try to avoid changing whitespace far away from your real changes. I have a very strong hatred for trailing whitespace because it breaks the {} keys in Vim and going to the end of the line. But I agree, it makes the diff bigger. http://codereview.appspot.com/71042/diff/5013/5020 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/5013/5020#newcode594 Line 594: bool DefaultJITMemoryManager::CheckInvariants() { On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > I would give this either an out string parameter or a string result type so that > you can describe why the invariant check failed. If you were to use a string > result type, you'd use empty to mark success. Done. http://codereview.appspot.com/71042/diff/5013/5020#newcode596 Line 596: I != Blocks.end(); ++I) { On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > Chris hates this. Always cache the .end() result in an E variable declared in > the for loop setup. Done. http://codereview.appspot.com/71042/diff/5013/5020#newcode598 Line 598: uint8_t *start = (uint8_t*)I->base(); On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > Stylistically, LLVM capitalizes its local variables. (I think they're crazy, but > consistency wins. :) Gwoss. Done. http://codereview.appspot.com/71042/diff/5013/5020#newcode601 Line 601: MemoryRangeHeader *lastHdr = NULL; On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > In C++, always try to declare variables where they're initialized. This needs to be declared outside the loop. The ordering in this patch was wrong. http://codereview.appspot.com/71042/diff/5013/5020#newcode610 Line 610: while (iter != head) { On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > Use a SmallPtrSet to store the set of free blocks to avoid the O(N^3) loops > here. Done. http://codereview.appspot.com/71042/diff/5013/5020#newcode624 Line 624: return false; // Bad Next pointer. On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > Why doesn't this always return false when there's more than one block? It seems > like blocks are guaranteed not to overlap, so there will always be at least one > block for which iter doesn't fall between the start and the end. That is a good point, and the fact that it didn't fail suggests that this method doesn't really work. :( http://codereview.appspot.com/71042/diff/5013/5020#newcode645 Line 645: hdr = &hdr->getBlockAfter(); On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > How do you know that the block after hdr is valid? Can't it have fallen off the > end of the slab? The problem is that I did the check after going to the next block. These two lines should come after the check, and the LastHdr code should just be a simple remembering-of-the-last-element. Fixed. http://codereview.appspot.com/71042/diff/5013/5020#newcode646 Line 646: if (lastHdr && lastHdr->ThisAllocated != hdr->PrevAllocated) { On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > If lastHdr is NULL here, you would have crashed earlier in accessing > hdr->ThisAllocated, etc. Do you mean to check hdr? No, because it's the address > of a reference. What's going on? See last comment. http://codereview.appspot.com/71042/diff/5013/5015 File unittests/ExecutionEngine/JIT/JITMemoryManagerTest.cpp (right): http://codereview.appspot.com/71042/diff/5013/5015#newcode27 Line 27: JITMemoryManager *MemMgr = JITMemoryManager::CreateDefaultMemManager(); On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > Use an OwningPtr here: > http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/OwningPtr.h?v... Done. http://codereview.appspot.com/71042/diff/5013/5015#newcode38 Line 38: TEST(JITMemoryManagerTest, ThreeFunction) { On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > What is this checking that the OneFunction test didn't? Try to name the test > according to what it's checking. Done. I just got rid of OneFunction. http://codereview.appspot.com/71042/diff/5013/5015#newcode81 Line 81: // Make three large functions and then try and allocate three small functions. On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > Mention why this is significant. Are you trying to get the small functions to be > allocated inside the slabs for the large functions? If so, how do you know > they're actually getting allocated there? Yup, that's what I'm trying to exercise. I hacked in another method to the JITMemoryManager interface so I could make that assertion. http://codereview.appspot.com/71042/diff/5013/5015#newcode88 Line 88: uintptr_t bigFuncSize = MemMgr->GetNewBlockSize() - (2 * 1024); On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > Make these const so that readers know they don't change accidentally. Done. http://codereview.appspot.com/71042/diff/5013/5015#newcode91 Line 91: // one On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > We don't really need a comment telling us that the next line is going to say, > "F1". We can just look at the next line. It's to visually break up the copy-pasted block. I'll replace it with another kind of separator. I could use a loop instead, but I feel like the mental overhead of getting these objects into a vector is probably not worth it. http://codereview.appspot.com/71042/diff/5013/5015#newcode135 Line 135: // deallocate two On 2009/06/16 18:04:03, Jeffrey Yasskin wrote: > This comment looks like you're trying to deallocate two of the functions, not > function #2. The "function #2" interpretation actually doesn't need a comment, > since the next line says, deallocateMemForFunction(F2). It was mostly there to make it obvious that they're being deallocated out of order. Instead of rewriting the number I just commented that they're out of order.
Sign in to reply to this message.
Mostly LGTM. Nick, could you take a look from an LLVM perspective? http://codereview.appspot.com/71042/diff/5023/5024 File include/llvm/ExecutionEngine/JITMemoryManager.h (right): http://codereview.appspot.com/71042/diff/5023/5024#newcode138 Line 138: /// CheckInvariants - For testing only. Return true if all of the internal "Return the empty string if all the internal invariants are preserved, and a helpful error message otherwise." http://codereview.appspot.com/71042/diff/5023/5030 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/5023/5030#newcode276 Line 276: unsigned NewBlockSize; This is really just the minimum or maybe default size, since larger functions can allocate larger slabs. http://codereview.appspot.com/71042/diff/5023/5030#newcode278 Line 278: static const unsigned StubBlockSize; Probably needs a comment. http://codereview.appspot.com/71042/diff/5023/5030#newcode648 Line 648: while (Start <= (uint8_t*)Hdr && (uint8_t*)Hdr < End){ I would probably make this a for loop: for (MemoryRangeHeader *Hdr = (MemoryRangeHeader*)Start; Start <= (uint8_t*)Hdr && (uint8_t*)Hdr < End; Hdr = &Hdr->getBlockAfter()) {... http://codereview.appspot.com/71042/diff/5023/5030#newcode661 Line 661: return "End of block size marker and BlockSize do not match."; One reason to return a string is that you can put helpful information in it: string Result; raw_string_ostream OS; OS << "End of block size marker (" << *Marker << ") and BlockSize (" << Hdr->BlockSize << ") don't match."; OS.flush(); return Result; http://codereview.appspot.com/71042/diff/5023/5025 File unittests/ExecutionEngine/JIT/JITMemoryManagerTest.cpp (right): http://codereview.appspot.com/71042/diff/5023/5025#newcode37 Line 37: Function *F1 = makeFakeFunction(); Similarly, use OwningPtr<>s for the Functions too. You shouldn't have explicit deletes in most functions.
Sign in to reply to this message.
http://codereview.appspot.com/71042/diff/5023/5024 File include/llvm/ExecutionEngine/JITMemoryManager.h (right): http://codereview.appspot.com/71042/diff/5023/5024#newcode140 Line 140: virtual std::string CheckInvariants() { Why on earth does this return a string? http://codereview.appspot.com/71042/diff/5023/5029 File lib/ExecutionEngine/JIT/JITEmitter.cpp (right): http://codereview.appspot.com/71042/diff/5023/5029#newcode1015 Line 1015: Don't include unrelated whitespace changes when you mail it to llvm-commits. http://codereview.appspot.com/71042/diff/5023/5029#newcode1270 Line 1270: MinSize = (uintptr_t)(2 * (BufferEnd - BufferBegin)); Your description of MinSize was: "When reattempting to JIT a function after running out of space, we store the minimum size for the new allocation here. When we successfully emit the function, we reset this back to zero so it doesn't keep growing." It seems that this is really storing the next size to try when allocating the buffer, which isn't really the minimum of anything. Please fix both the variable name and the comment. http://codereview.appspot.com/71042/diff/5023/5030 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/5023/5030#newcode338 Line 338: uint8_t *MemBase = static_cast<uint8_t*>(B.base()); Please just use char* if you plan to do pointer arithmetic. I realize this file has both. http://codereview.appspot.com/71042/diff/5023/5030#newcode342 Line 342: MemoryRangeHeader *endBlock = (MemoryRangeHeader*)(MemBase + B.size())-1; Did you really mean to put the -1 outside the cast? Be consistent about whether you use C-style or C++ style casts. I think either is actually fine if you're only casting to or from char*. http://codereview.appspot.com/71042/diff/5023/5030#newcode352 Line 352: newBlock->BlockSize = (int8_t*)endBlock - (int8_t*)newBlock; Again, char*. Possibly C++ casts. http://codereview.appspot.com/71042/diff/5023/5030#newcode481 Line 481: NewBlockSize = 4 << 20; Any reason not to use an initializer list here? http://codereview.appspot.com/71042/diff/5023/5030#newcode512 Line 512: Mem3->BlockSize = sizeof(MemoryRangeHeader); I don't understand why you changed this (and again for Mem1->BlockSize). Could you explain? http://codereview.appspot.com/71042/diff/5023/5030#newcode601 Line 601: std::string DefaultJITMemoryManager::CheckInvariants() { return void. http://codereview.appspot.com/71042/diff/5023/5030#newcode616 Line 616: if (Found) { You can eliminate this by changing your for loop condition to "I != E && !Found". It's an occasional idiom in LLVM. http://codereview.appspot.com/71042/diff/5023/5030#newcode621 Line 621: return "Corrupt FreeMemoryList."; assert(Found && "Corrupt FreeMemoryList."); http://codereview.appspot.com/71042/diff/5023/5030#newcode624 Line 624: if (FreeRange->Next->Prev != FreeRange) { assert. http://codereview.appspot.com/71042/diff/5023/5030#newcode651 Line 651: if (!FreeHdrSet.count(Hdr)) { assert. http://codereview.appspot.com/71042/diff/5023/5030#newcode657 Line 657: if (!(Start <= (uint8_t*)Marker && (uint8_t*)Marker < End)) { assert. http://codereview.appspot.com/71042/diff/5023/5030#newcode660 Line 660: if (Hdr->BlockSize != *Marker) { assert. http://codereview.appspot.com/71042/diff/5023/5030#newcode661 Line 661: return "End of block size marker and BlockSize do not match."; On 2009/06/17 01:59:53, Jeffrey Yasskin wrote: > One reason to return a string is that you can put helpful information in it: > > string Result; > raw_string_ostream OS; > OS << "End of block size marker (" << *Marker << ") and BlockSize (" > << Hdr->BlockSize << ") don't match."; > OS.flush(); > return Result; Sure. If you want to include such useful information then instead of using assert, use cerr << ... followed by abort(). http://codereview.appspot.com/71042/diff/5023/5030#newcode663 Line 663: } else if (Hdr->ThisAllocated != 1) { assert. http://codereview.appspot.com/71042/diff/5023/5030#newcode668 Line 668: return "Allocation flags are inconsistent."; assert. http://codereview.appspot.com/71042/diff/5023/5030#newcode670 Line 670: return "The first header should have PrevAllocated true."; assert. http://codereview.appspot.com/71042/diff/5023/5025 File unittests/ExecutionEngine/JIT/JITMemoryManagerTest.cpp (right): http://codereview.appspot.com/71042/diff/5023/5025#newcode1 Line 1: //===- llvm/unittest/VMCore/Metadata.cpp - Metadata unit tests ------------===// No it's not. :) http://codereview.appspot.com/71042/diff/5023/5025#newcode23 Line 23: FunctionType *fnTy = FunctionType::get(Type::VoidTy, params, false); The prevailing naming convention would call this "const FunctionType *FTy". At the very least add the const.
Sign in to reply to this message.
http://codereview.appspot.com/71042/diff/5023/5024 File include/llvm/ExecutionEngine/JITMemoryManager.h (right): http://codereview.appspot.com/71042/diff/5023/5024#newcode138 Line 138: /// CheckInvariants - For testing only. Return true if all of the internal On 2009/06/17 01:59:53, Jeffrey Yasskin wrote: > "Return the empty string if all the internal invariants are preserved, and a > helpful error message otherwise." Done. http://codereview.appspot.com/71042/diff/5023/5024#newcode140 Line 140: virtual std::string CheckInvariants() { On 2009/06/17 04:53:57, nlewycky wrote: > Why on earth does this return a string? It's used for testing. I'd like to write the method in JITMemoryManagerTest.cpp, but then I'd have to find a way to expose the MemoryRangeHeader structs and all kinds of craziness. I figured it would just be easier to do all the checks I wanted in this method and return the result. http://codereview.appspot.com/71042/diff/5023/5029 File lib/ExecutionEngine/JIT/JITEmitter.cpp (right): http://codereview.appspot.com/71042/diff/5023/5029#newcode1015 Line 1015: On 2009/06/17 04:53:57, nlewycky wrote: > Don't include unrelated whitespace changes when you mail it to llvm-commits. Done. http://codereview.appspot.com/71042/diff/5023/5029#newcode1270 Line 1270: MinSize = (uintptr_t)(2 * (BufferEnd - BufferBegin)); On 2009/06/17 04:53:57, nlewycky wrote: > Your description of MinSize was: > > "When reattempting to JIT a function after running out of space, we store the > minimum size for the new allocation here. When we successfully emit the > function, we reset this back to zero so it doesn't keep growing." > > It seems that this is really storing the next size to try when allocating the > buffer, which isn't really the minimum of anything. Please fix both the variable > name and the comment. Name and comment fixed. Maybe a better way to do this would be to track how much space is required in the MachineCodeEmitter base class, so that when we retry we know exactly how much space we need. http://codereview.appspot.com/71042/diff/5023/5030 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/5023/5030#newcode276 Line 276: unsigned NewBlockSize; On 2009/06/17 01:59:53, Jeffrey Yasskin wrote: > This is really just the minimum or maybe default size, since larger functions > can allocate larger slabs. Appropriately renamed to DefaultSlabSize. http://codereview.appspot.com/71042/diff/5023/5030#newcode278 Line 278: static const unsigned StubBlockSize; On 2009/06/17 01:59:53, Jeffrey Yasskin wrote: > Probably needs a comment. Done. http://codereview.appspot.com/71042/diff/5023/5030#newcode338 Line 338: uint8_t *MemBase = static_cast<uint8_t*>(B.base()); On 2009/06/17 04:53:57, nlewycky wrote: > Please just use char* if you plan to do pointer arithmetic. I realize this file > has both. Done. This code actually came out of the constructor. http://codereview.appspot.com/71042/diff/5023/5030#newcode342 Line 342: MemoryRangeHeader *endBlock = (MemoryRangeHeader*)(MemBase + B.size())-1; On 2009/06/17 04:53:57, nlewycky wrote: > Did you really mean to put the -1 outside the cast? Yes, I need enough space to write the header info. > Be consistent about whether you use C-style or C++ style casts. I think either > is actually fine if you're only casting to or from char*. Do you mind if I consistently use C casts? The only static_cast I see is in the constructor. http://codereview.appspot.com/71042/diff/5023/5030#newcode352 Line 352: newBlock->BlockSize = (int8_t*)endBlock - (int8_t*)newBlock; On 2009/06/17 04:53:57, nlewycky wrote: > Again, char*. Possibly C++ casts. Actually BlockSize is uintptr_t so I'll cast to that. http://codereview.appspot.com/71042/diff/5023/5030#newcode481 Line 481: NewBlockSize = 4 << 20; On 2009/06/17 04:53:57, nlewycky wrote: > Any reason not to use an initializer list here? I made this a static constant now, so it's not relevant anymore. http://codereview.appspot.com/71042/diff/5023/5030#newcode512 Line 512: Mem3->BlockSize = sizeof(MemoryRangeHeader); On 2009/06/17 04:53:57, nlewycky wrote: > I don't understand why you changed this (and again for Mem1->BlockSize). Could > you explain? It was a bug in the original code. If you look at the comment describing BlockSize, BlockSize should *include* the size of the header. I ran into this problem because in CheckInvariants I iterate over the ranges by calling getBlockAfter(), which when the BlockSize is zero, returns a reference to the same header. http://codereview.appspot.com/71042/diff/5023/5030#newcode601 Line 601: std::string DefaultJITMemoryManager::CheckInvariants() { On 2009/06/17 04:53:57, nlewycky wrote: > return void. If I did that, then I wouldn't be able to use the gtest ASSERT/EXPECT macros in JITMemoryManagerTest.cpp. Instead of a test failure, I'd get a test abort, which is not what I want. http://codereview.appspot.com/71042/diff/5023/5030#newcode616 Line 616: if (Found) { On 2009/06/17 04:53:57, nlewycky wrote: > You can eliminate this by changing your for loop condition to "I != E && > !Found". It's an occasional idiom in LLVM. Done. http://codereview.appspot.com/71042/diff/5023/5030#newcode648 Line 648: while (Start <= (uint8_t*)Hdr && (uint8_t*)Hdr < End){ On 2009/06/17 01:59:53, Jeffrey Yasskin wrote: > I would probably make this a for loop: > for (MemoryRangeHeader *Hdr = (MemoryRangeHeader*)Start; > Start <= (uint8_t*)Hdr && (uint8_t*)Hdr < End; > Hdr = &Hdr->getBlockAfter()) {... Done. http://codereview.appspot.com/71042/diff/5023/5030#newcode661 Line 661: return "End of block size marker and BlockSize do not match."; On 2009/06/17 01:59:53, Jeffrey Yasskin wrote: > One reason to return a string is that you can put helpful information in it: > > string Result; > raw_string_ostream OS; > OS << "End of block size marker (" << *Marker << ") and BlockSize (" > << Hdr->BlockSize << ") don't match."; > OS.flush(); > return Result; Done. http://codereview.appspot.com/71042/diff/5023/5025 File unittests/ExecutionEngine/JIT/JITMemoryManagerTest.cpp (right): http://codereview.appspot.com/71042/diff/5023/5025#newcode1 Line 1: //===- llvm/unittest/VMCore/Metadata.cpp - Metadata unit tests ------------===// On 2009/06/17 04:53:57, nlewycky wrote: > No it's not. :) Done. http://codereview.appspot.com/71042/diff/5023/5025#newcode23 Line 23: FunctionType *fnTy = FunctionType::get(Type::VoidTy, params, false); On 2009/06/17 04:53:57, nlewycky wrote: > The prevailing naming convention would call this "const FunctionType *FTy". At > the very least add the const. Done. http://codereview.appspot.com/71042/diff/5023/5025#newcode37 Line 37: Function *F1 = makeFakeFunction(); On 2009/06/17 01:59:53, Jeffrey Yasskin wrote: > Similarly, use OwningPtr<>s for the Functions too. You shouldn't have explicit > deletes in most functions. I didn't know if that would work, but I think I get it now. Done.
Sign in to reply to this message.
I updated this patch to deal with globals and other things. Could you guys take another look? Thanks.
Sign in to reply to this message.
http://codereview.appspot.com/71042/diff/8001/9002 File include/llvm/CodeGen/JITCodeEmitter.h (right): http://codereview.appspot.com/71042/diff/8001/9002#newcode260 Line 260: Result = 0; // NULL ptr here! If you really want to mention NULL here, just say "Result = NULL". The comment doesn't seem to give any more information. http://codereview.appspot.com/71042/diff/8001/9001 File include/llvm/ExecutionEngine/JITMemoryManager.h (right): http://codereview.appspot.com/71042/diff/8001/9001#newcode128 Line 128: /// cannot be called between calls to startFunctionBody and endFunctionBody. The implementation of this method needs to assert this. http://codereview.appspot.com/71042/diff/8001/9007 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (left): http://codereview.appspot.com/71042/diff/8001/9007#oldcode343 Line 343: /// this method does not touch the current block and can be called at any If this comment was useful before, and allocateStub and allocateGlobal still differ from allocateSpace, you should probably propagate it into the new file. http://codereview.appspot.com/71042/diff/8001/9007 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/8001/9007#newcode18 Line 18: #include "llvm/Support/Debug.h" I think you need to #define DEBUG_TYPE to something, like in JITEmitter.cpp. http://codereview.appspot.com/71042/diff/8001/9007#newcode259 Line 259: private: LLVM usually omits this first "private:" in classes. http://codereview.appspot.com/71042/diff/8001/9007#newcode291 Line 291: /// AlignPtr - Align Ptr to Alignment bytes, rounding down. So, AlignPtr(7, 4)==4, not 8? I think this function should match the semantics of std::align, defined in 20.8.11 of http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2009/n2914.pdf : void *align(std::size_t alignment, std::size_t size, void *&ptr, std::size_t& space); 1 Effects: If it is possible to fit size bytes of storage aligned by alignment into the buffer pointed to by ptr with length space, the function updates ptr to point to the first possible address of such storage and decreases space by the number of bytes used for alignment. Otherwise, the function does nothing. 2 Requires: — alignment shall be a fundamental alignment value or an extended alignment value supported by the implementation in this context — ptr shall point to contiguous storage of at least space bytes 3 Returns: a null pointer if the requested aligned buffer would not fit into the available space, otherwise the adjusted value of ptr. 4 [ Note: the function updates its ptr and space arguments so that it can be called repeatedly with possibly different alignment and size arguments for the same buffer. http://codereview.appspot.com/71042/diff/8001/9007#newcode296 Line 296: ~(uintptr_t)(Alignment - 1)); For this to work, Alignment has to be a power of two, so you should assert() that. http://codereview.appspot.com/71042/diff/8001/9007#newcode398 Line 398: /// allocateNewCodeSlab - Helper method to allocate a new chunk of memory s/chunk/slab/g http://codereview.appspot.com/71042/diff/8001/9007#newcode403 Line 403: // two MemoryRangeHeaders, and allocate at least that much space. Why two MemoryRangeHeaders? You say say that one's for the user's block, and the other one's for a the fake allocated block at the end of the slab. http://codereview.appspot.com/71042/diff/8001/9007#newcode445 Line 445: /// cannot be called between calls to startFunctionBody and endFunctionBody. Check that. http://codereview.appspot.com/71042/diff/8001/9007#newcode573 Line 573: // Allocate the aligned space, going backwards from CurPtr. This needs to be documented in the class's comment, not just in AllocateSpace. http://codereview.appspot.com/71042/diff/8001/9007#newcode577 Line 577: if (Ptr < Base) { I would reverse this if() and return early when it succeeds. http://codereview.appspot.com/71042/diff/8001/9007#newcode604 Line 604: : PoisonMemory(POISON), LastSlab(0), StubAllocator(this), DataAllocator(this) Break the line after a comma, rather than before the '{'. http://codereview.appspot.com/71042/diff/8001/9007#newcode604 Line 604: : PoisonMemory(POISON), LastSlab(0), StubAllocator(this), DataAllocator(this) You can assign PoisonMemory inside the constructor instead of in the initializer list if it makes dealing with NDEBUG easier. http://codereview.appspot.com/71042/diff/8001/9007#newcode724 Line 724: return Result; This won't necessarily have flushed the ostream by the time it returns. http://codereview.appspot.com/71042/diff/8001/9007#newcode785 Line 785: return ""; // Empty string means truth. s/truth/everything is ok/ http://codereview.appspot.com/71042/diff/8001/9007#newcode792 Line 792: // Allocate memory for code in 64K slabs. 64K? That's a lot smaller than the current allocator. http://codereview.appspot.com/71042/diff/8001/9004 File lib/Target/TargetMachine.cpp (left): http://codereview.appspot.com/71042/diff/8001/9004#oldcode240 Line 240: Revert this nearly-unchanged file. http://codereview.appspot.com/71042/diff/8001/9003 File unittests/ExecutionEngine/JIT/JITMemoryManagerTest.cpp (right): http://codereview.appspot.com/71042/diff/8001/9003#newcode260 Line 260: //EXPECT_EQ(1U, MemMgr->GetNumDataSlabs()); These shouldn't be commented out. http://codereview.appspot.com/71042/diff/8001/9003#newcode262 Line 262: // After allocating a bunch of globals, we should have two. s/globals/stubs/
Sign in to reply to this message.
I'll get started running the nightly tests on this. http://codereview.appspot.com/71042/diff/8001/9002 File include/llvm/CodeGen/JITCodeEmitter.h (right): http://codereview.appspot.com/71042/diff/8001/9002#newcode260 Line 260: Result = 0; // NULL ptr here! On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > If you really want to mention NULL here, just say "Result = NULL". The comment > doesn't seem to give any more information. That must be left over from when I was debugging. I was really confused why something was returning NULL, and it took me a long time to track it down until I found it here. Deleted. http://codereview.appspot.com/71042/diff/8001/9001 File include/llvm/ExecutionEngine/JITMemoryManager.h (right): http://codereview.appspot.com/71042/diff/8001/9001#newcode128 Line 128: /// cannot be called between calls to startFunctionBody and endFunctionBody. On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > The implementation of this method needs to assert this. There's no easy way to assert this with the information that the JITMemoryManager has right now. So far as I can tell, the only client of this method is ExecutionEngine::emitGlobals, because all other calls through JITEmitter::allocateSpace occur between function starts and ends. The only client of emitGlobals is the interpreter, which doesn't call this method. So far as I can tell, this method is dead code, but some client might be using it with their custom memory manager. Is it worth adding a boolean field to the class just to track this to support a method that's basically dead code? http://codereview.appspot.com/71042/diff/8001/9007 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (left): http://codereview.appspot.com/71042/diff/8001/9007#oldcode343 Line 343: /// this method does not touch the current block and can be called at any On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > If this comment was useful before, and allocateStub and allocateGlobal still > differ from allocateSpace, you should probably propagate it into the new file. allocateSpace is the broken method that nobody uses, so I figured it would be better to move this piece of documentation into its docstring. http://codereview.appspot.com/71042/diff/8001/9007 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/8001/9007#newcode18 Line 18: #include "llvm/Support/Debug.h" On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > I think you need to #define DEBUG_TYPE to something, like in JITEmitter.cpp. > Done. http://codereview.appspot.com/71042/diff/8001/9007#newcode259 Line 259: private: On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > LLVM usually omits this first "private:" in classes. Done. http://codereview.appspot.com/71042/diff/8001/9007#newcode291 Line 291: /// AlignPtr - Align Ptr to Alignment bytes, rounding down. On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > So, AlignPtr(7, 4)==4, not 8? I think this function should match the semantics > of std::align, defined in 20.8.11 of > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2009/n2914.pdf : > > void *align(std::size_t alignment, std::size_t size, > void *&ptr, std::size_t& space); > 1 Effects: If it is possible to fit size bytes of storage aligned by alignment > into the buffer pointed to by > ptr with length space, the function updates ptr to point to the first possible > address of such storage > and decreases space by the number of bytes used for alignment. Otherwise, the > function does nothing. > 2 Requires: > — alignment shall be a fundamental alignment value or an extended alignment > value supported by > the implementation in this context > — ptr shall point to contiguous storage of at least space bytes > 3 Returns: a null pointer if the requested aligned buffer would not fit into the > available space, otherwise > the adjusted value of ptr. > 4 [ Note: the function updates its ptr and space arguments so that it can be > called repeatedly with > possibly different alignment and size arguments for the same buffer. > What you're saying is that the allocator go forwards, not backwards. I agree, that makes more sense. Done. http://codereview.appspot.com/71042/diff/8001/9007#newcode296 Line 296: ~(uintptr_t)(Alignment - 1)); On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > For this to work, Alignment has to be a power of two, so you should assert() > that. Done. http://codereview.appspot.com/71042/diff/8001/9007#newcode398 Line 398: /// allocateNewCodeSlab - Helper method to allocate a new chunk of memory On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > s/chunk/slab/g Done. http://codereview.appspot.com/71042/diff/8001/9007#newcode403 Line 403: // two MemoryRangeHeaders, and allocate at least that much space. On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > Why two MemoryRangeHeaders? You say say that one's for the user's block, and the > other one's for a the fake allocated block at the end of the slab. Done. http://codereview.appspot.com/71042/diff/8001/9007#newcode445 Line 445: /// cannot be called between calls to startFunctionBody and endFunctionBody. On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > Check that. Not done, as discussed. http://codereview.appspot.com/71042/diff/8001/9007#newcode573 Line 573: // Allocate the aligned space, going backwards from CurPtr. On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > This needs to be documented in the class's comment, not just in AllocateSpace. Done. http://codereview.appspot.com/71042/diff/8001/9007#newcode577 Line 577: if (Ptr < Base) { On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > I would reverse this if() and return early when it succeeds. Done. http://codereview.appspot.com/71042/diff/8001/9007#newcode604 Line 604: : PoisonMemory(POISON), LastSlab(0), StubAllocator(this), DataAllocator(this) On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > You can assign PoisonMemory inside the constructor instead of in the initializer > list if it makes dealing with NDEBUG easier. I guess that's OK since it's not const anyway. OTOH, when Nick reviewed this, he asked about moving things into the initializer list. Whatever, I'm doing it. http://codereview.appspot.com/71042/diff/8001/9007#newcode724 Line 724: return Result; On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > This won't necessarily have flushed the ostream by the time it returns. Fixed, and also below. http://codereview.appspot.com/71042/diff/8001/9007#newcode785 Line 785: return ""; // Empty string means truth. On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > s/truth/everything is ok/ Done. http://codereview.appspot.com/71042/diff/8001/9007#newcode792 Line 792: // Allocate memory for code in 64K slabs. On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > 64K? That's a lot smaller than the current allocator. Yeah, but now we have a reasonable allocator, and I figure this will make LLVM friendlier for embedded platforms. I'm going to make it 512K. http://codereview.appspot.com/71042/diff/8001/9004 File lib/Target/TargetMachine.cpp (left): http://codereview.appspot.com/71042/diff/8001/9004#oldcode240 Line 240: On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > Revert this nearly-unchanged file. Done. http://codereview.appspot.com/71042/diff/8001/9003 File unittests/ExecutionEngine/JIT/JITMemoryManagerTest.cpp (right): http://codereview.appspot.com/71042/diff/8001/9003#newcode260 Line 260: //EXPECT_EQ(1U, MemMgr->GetNumDataSlabs()); On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > These shouldn't be commented out. Done. http://codereview.appspot.com/71042/diff/8001/9003#newcode262 Line 262: // After allocating a bunch of globals, we should have two. On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > s/globals/stubs/ Done.
Sign in to reply to this message.
LGTM. If llvm-commits hasn't gotten back to you by the end of tomorrow, go ahead and commit this to the Unladen Swallow tree on Wednesday. http://codereview.appspot.com/71042/diff/8001/9001 File include/llvm/ExecutionEngine/JITMemoryManager.h (right): http://codereview.appspot.com/71042/diff/8001/9001#newcode128 Line 128: /// cannot be called between calls to startFunctionBody and endFunctionBody. On 2009/07/13 22:18:00, Reid Kleckner wrote: > On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > > The implementation of this method needs to assert this. > > There's no easy way to assert this with the information that the > JITMemoryManager has right now. > > So far as I can tell, the only client of this method is > ExecutionEngine::emitGlobals, because all other calls through > JITEmitter::allocateSpace occur between function starts and ends. The only > client of emitGlobals is the interpreter, which doesn't call this method. So > far as I can tell, this method is dead code, but some client might be using it > with their custom memory manager. > > Is it worth adding a boolean field to the class just to track this to support a > method that's basically dead code? Not in this change, anyway. And we discovered that endFunction has the relevant assertion, so never mind. http://codereview.appspot.com/71042/diff/8001/9007 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/8001/9007#newcode604 Line 604: : PoisonMemory(POISON), LastSlab(0), StubAllocator(this), DataAllocator(this) On 2009/07/13 22:18:00, Reid Kleckner wrote: > On 2009/07/10 17:52:59, Jeffrey Yasskin wrote: > > You can assign PoisonMemory inside the constructor instead of in the > initializer > > list if it makes dealing with NDEBUG easier. > > I guess that's OK since it's not const anyway. OTOH, when Nick reviewed this, > he asked about moving things into the initializer list. Whatever, I'm doing it. Putting things in the initializer list is good, unless it makes things harder to read overall. Of course, the LLVM folks beat me in any disagreements over "harder to read". http://codereview.appspot.com/71042/diff/9010/9017#newcode295 Line 295: return (uint8_t*)(((uintptr_t)Ptr) & You should still document the direction that AlignPtr modifies Ptr, probably with an example or two. http://codereview.appspot.com/71042/diff/9010/9017#newcode412 Line 412: (MemoryRangeHeader*)(MemBase + B.size()) - 1; s/in user's/in the user's/ http://codereview.appspot.com/71042/diff/9010/9017#newcode734 Line 734: } while (FreeRange != FreeHead); Unfortunately, this breaks the named return value optimization. For CheckInvariants it may not matter, but it's better to return the local variable directly by name. Just call OS.flush().
Sign in to reply to this message.
http://codereview.appspot.com/71042/diff/9010/9017 File lib/ExecutionEngine/JIT/JITMemoryManager.cpp (right): http://codereview.appspot.com/71042/diff/9010/9017#newcode295 Line 295: /// AlignPtr - Align Ptr to Alignment bytes, rounding down. On 2009/07/13 22:33:00, Jeffrey Yasskin wrote: > You should still document the direction that AlignPtr modifies Ptr, probably > with an example or two. Done. http://codereview.appspot.com/71042/diff/9010/9017#newcode412 Line 412: // two MemoryRangeHeaders: the one in user's block, and the one at the end On 2009/07/13 22:33:00, Jeffrey Yasskin wrote: > s/in user's/in the user's/ Done. http://codereview.appspot.com/71042/diff/9010/9017#newcode734 Line 734: return OS.str(); On 2009/07/13 22:33:00, Jeffrey Yasskin wrote: > Unfortunately, this breaks the named return value optimization. For > CheckInvariants it may not matter, but it's better to return the local variable > directly by name. Just call OS.flush(). Relying on known compiler optimizations feels sketchy to me. Done.
Sign in to reply to this message.
Please take a look at my attempt to merge the bump allocators.
Sign in to reply to this message.
Looks decent http://codereview.appspot.com/71042/diff/12011/12014 File include/llvm/Support/Allocator.h (left): http://codereview.appspot.com/71042/diff/12011/12014#oldcode52 Line 52: void *TheMemory; You may get pushback for moving the struct definition into the header, but I don't see a great reason not to do it. http://codereview.appspot.com/71042/diff/12011/12014 File include/llvm/Support/Allocator.h (right): http://codereview.appspot.com/71042/diff/12011/12014#newcode62 Line 62: virtual sys::MemoryBlock Allocate(size_t Size, size_t Alignment); This should just forward to the MallocAllocator, I think. http://codereview.appspot.com/71042/diff/12011/12014#newcode83 Line 83: /// malloc or mmap. It's not going to be malloc or mmap themselves; it's a wrapper around them. http://codereview.appspot.com/71042/diff/12011/12021 File lib/Support/Allocator.cpp (right): http://codereview.appspot.com/71042/diff/12011/12021#newcode133 Line 133: Base = malloc(Size + Alignment - 1); posix_memalign is available to allocate aligned memory. http://codereview.appspot.com/71042/diff/12011/12021#newcode134 Line 134: Base = BumpPtrAllocator::AlignPtr((char*)Base, Alignment); If this case runs and adjusts the pointer, MallocSlabAllocator::Free() can crash, since you're only allowed to pass the exact result of malloc() to free().
Sign in to reply to this message.
Thanks for the review! http://codereview.appspot.com/71042/diff/12011/12014 File include/llvm/Support/Allocator.h (right): http://codereview.appspot.com/71042/diff/12011/12014#newcode62 Line 62: virtual sys::MemoryBlock Allocate(size_t Size, size_t Alignment); On 2009/07/20 18:12:59, Jeffrey Yasskin wrote: > This should just forward to the MallocAllocator, I think. Done. http://codereview.appspot.com/71042/diff/12011/12014#newcode83 Line 83: /// malloc or mmap. On 2009/07/20 18:12:59, Jeffrey Yasskin wrote: > It's not going to be malloc or mmap themselves; it's a wrapper around them. Done. http://codereview.appspot.com/71042/diff/12011/12021 File lib/Support/Allocator.cpp (right): http://codereview.appspot.com/71042/diff/12011/12021#newcode134 Line 134: Base = BumpPtrAllocator::AlignPtr((char*)Base, Alignment); On 2009/07/20 18:12:59, Jeffrey Yasskin wrote: > If this case runs and adjusts the pointer, MallocSlabAllocator::Free() can > crash, since you're only allowed to pass the exact result of malloc() to free(). Ach, true! In any case, now that I forward to malloc allocator, this code is gone.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
