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

[Adhemerval Zanella]

On 09/17/2012 06:58 PM, Jim Grosbach wrote:
> Hello,
>
> First off, this is awesome to see PPC getting MC-ified. Thank you for working on this!
>
> I don't know PPC well enough to comment on the architecture specific aspects of this, but a few general comments inline below:

Thanks for the comments Jim, considerations below.


>
> Is StubAddr guaranteed to be 32-bit aligned? Typically not.

Although it is not guaranteed to be 32-bit aligned, I believe the way both relocation
and the stub creation are done is with the assumption higher level objects knows how
to place the data and call the stub creation and relocation methods with correct memory 
arguments. For instance, if it calls the stub creation with a non 32-bits aligned memory,
should the method just align it and possible overwriting a valid previous instruction or
just fail?

Based on that I adjusted the relocations created on external function call to proper
call the relocation method with correct memory positions.


>
> LLVM style is to use the "->" operator here. e.g., "it->Name".

Fixed.

>
> Can omit "lets" here. If you prefer w/ it, it's missing an apostrophe (i.e., "let's").

Omitting 'lets'.

>
> Open brace goes on the same line as the close paren of the decl.

Fixed and also fixed in the previous function.

>
> The names for these functions are a) ambiguous and b) not following LLVM's naming guidelines. Please review: http://llvm.org/docs/CodingStandards.html#coding-standards

I have changed to 'applyPPCxxx' to follow the verb predicative in the documentation.


>
>
> LocalAddress is not guaranteed to be 16-bit aligned. Ditto on below references.

Same comment as for the stub creation.

In attachment the patch with some adjustment.



-- 
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: 0009-Initial-support-for-PowerPC64-MCJIT.patch
Type: text/x-patch
Size: 16274 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120918/4bcfc1bd/attachment.bin>