[llvm-commits] [PATCH 9/10] Initial support for PowerPC64 MCJIT
Adhemerval Zanella
azanella at linux.vnet.ibm.com
Thu Sep 20 07:56:32 PDT 2012
On 09/19/2012 04:45 PM, Hal Finkel wrote:
> On Wed, 19 Sep 2012 20:09:45 +0200
> Roman Divacky <rdivacky at freebsd.org> wrote:
>
>> Some comments below.
> I've added a few additional comments.
Thanks, I'll reply both in this message.
>
>>> diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
>>> b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp index
>>> a1c0e40..5742a1d 100644 ---
>>> a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp +++
>>> b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp @@ -27,6
>>> +27,14 @@ using namespace llvm::object;
>>> namespace {
>>>
>>> +static inline
>>> +error_code Check(error_code Err) {
> Should be 'check' by the coding convention.
Fixed. Just a note that I copied the 'Check' function from 'lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp'.
>>> + if (it->Name == ".got")
>>> + break;
>>> + if (it->Name == ".toc")
>>> + break;
>>> + if (it->Name == ".tocbss")
>>> + break;
>>> + if (it->Name == ".plt")
>>> + break;
>>> + }
>> Can you reflow this to use else if?
I used a or logic instead.
>> + assert (it != Sections.end());
>> + // Per the ppc64-elf-linux ABI, The TOC base if TOC value plus
> typo: if -> is
Fixed.
>>> 0x8000
>>> + // thus permitting a full 64 Kbytes segment.
>>> + return (*it).LoadAddress + 0x8000;
>> Use it->LoadAddress.
Fixed.
>> + SymbolRef TargetSymbol;
>> + Check(i->getSymbol(TargetSymbol));
>> + uint64_t TargetSymbolOffset;
>> + Check(i->getOffset(TargetSymbolOffset));
>> + int64_t TargetAdditionalInfo;
>> + Check(i->getAdditionalInfo(TargetAdditionalInfo));
>> Please declare all variables first and then Check all of them at once.
I believe it is better now.
>>> + case ELF::R_PPC64_ADDR14 : {
>>> + assert(((Value + Addend) & 3) == 0);
>>> + uint16_t *insn = ((uint16_t*)LocalAddress+1);
>>> + writeInt16BE(LocalAddress + 2, (*insn & 3) | ((Value + Addend)
>>> & 0xfffc));
>> Can you please add the comment here why +1 and +2? It's not obvious.
Done, I also changed the logic to access the memory using uint8_t instead of uint16_t.
>>> to it
>>> + resolveRelocation(Target, (uint64_t)Target,
>>> (uint64_t)Section.Address
>>> + + i->second, RelType, 0);
>>> + DEBUG(dbgs() << " Stub function found\n");
> Would it make sense to print the symbol name in this debugging
> statement?
>
>>> + } else {
>>> + // Create a new stub function.
>>> + DEBUG(dbgs() << " Create a new stub function\n");
> Same here; context would be useful.
The debug named symbol are already being printed in previous debug directives.
>>> + + Section.StubOffset, RelType, 0);
>>> + if (SymType == SymbolRef::ST_Unknown)
>>> + // Restore the TOC for external calls
>>> + *((uint32_t*)Target+1) = 0xE8410028; // ld r2,40(r1)
>> Is the alignment ok here as Jim Grosbnach mentioned?
Yes and fixed.
Patch in attachments
--
Adhemerval Zanella Netto
Software Engineer
Linux Technology Center Brazil
Toolchain / GLIBC on Power Architecture
azanella at linux.vnet.ibm.com / azanella at br.ibm.com
+55 61 8642-9890
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Initial-support-for-PowerPC64-MCJIT.patch
Type: text/x-patch
Size: 20484 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120920/eb0519e0/attachment.bin>
More information about the llvm-commits
mailing list