[llvm-commits] [PATCH 9/10] Initial support for PowerPC64 MCJIT

[Adhemerval Zanella]

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>