468445 - Nanojit support for PowerPC64

Status: VERIFIED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Edwin Smith]

32bit PPC port is done (for mac at least), now lets do PPC64

* requires general 64bit fixes in LIR
* new instructions for handling pointers
* optimizations, eg int32->double, uint32->double

Comment 1

[Brent Baker]

PPC 64bit buildbot slave has been added and will be activated once this is pushed.

Comment 2

[Edwin Smith]

work in progress, with hg branches for blocking patches

http://hg.mozilla.org/users/edwsmith_adobe.com/ppc64

Comment 3

[Edwin Smith]

Attachment: LIR change for 64bit and new support for PPC64

This is what remains after factoring out earlier patches.  It needs to be split into 2+ smaller patches, but I'm posting now for early feedback.

I am tempted to isolate the generic LIR 64bit changes from the PPC/x64 backend changes, even though the 64bit-specific opcodes won't be exersized yet (in tamarin, at least).  if someone has a better suggestion on how to factor this patch please chime in.

Comment 4

[Edwin Smith]

Attachment: 64bit upgrades for LIR notation (no new backends)

Add many more 64bit lir opcodes, and new arg types for CallInfo signatures

This patch just does the LIR groundwork for 64bit backends, without adding
any new backends.

- rename ARGSIZE_LO to ARGSIZE_I, add ARGSIZE_U.  these represent 32bit
  args, and the I and U are a hint for when they must be promoted to
  64bit values on a 64bit ABI.
- add ARGSIZE_P alias, for pointer arguments
- add ARGSIZE_V for functions that return void
- define new more detailed signatures for all the functions in vm_fops.h

CodegenLIR was heavily modified to distinguish between int32, uint32, and
pointer values.  For pointer values, aliased LIR ops are used to pick
either the 32bit or 64bit operation required.

values of type Atom are generally treated as pointers, except in the
few cases when we know what we're doing with them (eg creating a boolean
atom from a bool).

Verifier now calls emitPtrConst() for const pointers and atoms, instead
of calling emitIntConst().   MIR's emitIntConst() is now just a wrapper
for emitPtrConst().

new LIR opcodes required by Tamarin:
  LIR_qursh:  64bit unsigned right shift
  LIR_qirsh:  64bit signed right shift
  LIR_qaddp:  64bit non-cse pointer add
  LIR_qxor:   64bit xor
  LIR_i2q:    sign extend 32->64
  LIR_u2q:    zero extend 32->64
  LIR_qeq:    64bit int equals
  LIR_qle:    64bit signed <=
  LIR_qlt:    64bit signed <
  LIR_qge:    64bit signed >=
  LIR_qgt:    64bit signed >
  LIR_qule:   64bit unsigned <=
  LIR_qult:   64bit unsigned <
  LIR_quge:   64bit unsigned >=
  LIR_qugt:   64bit unsigned >

new ops that replace old ones
  LIR_iaddp replaces LIR_addp: 32bit pointer add
  LIR_qcall replaces LIR_callh on 64bit.  64bit GP call
  LIR_icall replaces LIR_call: 32bit GP call.

LIR_callh is still used on 32bit builds to represent the MSW 32bit
result from a 64bit call.  Not for use at all on 64bit builds

these ops are mutually exclusive (i used on 32bit builds and
q used on 64bit builds): LIR_[iq]alloc, LIR_[iq]call,
LIR_[iq]param.

isS24() macro had to be fixed to properly sign extend on 64bit
builds... a case of misplaced parens, it turns out.

Comment 5

[Edwin Smith]

Attachment: ppc64 backend, requires lir64.patch

supports 64bit lir instructions when NANOJIT_64BIT is defined, plus a few other 64bit tweaks required in the assembler.

Comment 6

[Rick Reitmaier]

Comment on attachment 357154 [details] [diff] [review]
64bit upgrades for LIR notation (no new backends)

- Nativei386/PPC.cpp - asm_promote TODO()

- CodegenLIR.cpp:1960 do we need to assert 
  for 64b builds that the args are aligned or
  is this already assumed in the emitter.

- CodegenLIR.cp:2169 do we even support/test
  a build without WRITE_BARRIERS?
  Maybe the #error should be moved to include
  other non-supported ifdefs
 
- CodegenLIR.cpp:3817 binaryIns(LIR_lt),  mind
  to double check this optimization, it looks a little odd,
  although I think its carried over verbatim from MIR
  so its probably correct.

- CodegenLIR.cpp:3959 is _save_eip really 32b 
  on 64b platforms?

- CodegenLIR:cpp:3983 was this a 32b bug also?

And a general comment for which I don't have a 
good answer, but it strikes me that we now have 
a LIR for which one is not sure what instructions
require support on 32b and 64b platforms.  

For example, LIR_qalloc and LIR_ialloc are both
available in 32b builds but only ialloc is used.
Others such as LIR_i/u2q and LIR_qlo are
even more ambiguous; should a 32bit back-end 
support them? 

Perhaps we can ifdef AVMPLUS_64BIT around
the LIR instructions themselves to distinguish 
what is needed for each of the builds.

Also, I didn't look at all instances but I expect that
Assembler should not be using any of the LIR aliases;
this would only throw more confusion to the issue.

Comment 7

[Edwin Smith]

(In reply to comment #6)
> (From update of attachment 357154 [details] [diff] [review])
> - Nativei386/PPC.cpp - asm_promote TODO()

never called for 32bit backends.  This patch (357154) does not yet enable either of the 64bit backends.

> - CodegenLIR.cpp:1960 do we need to assert 
>   for 64b builds that the args are aligned or
>   is this already assumed in the emitter.

This is code that generates args for the VM abi, and the vm assumes everywhere that they're 8-byte aligned.  the semantics of LIR_alloc guarantee 8-byte alignment for any size >= 8 bytes.  (an assert would be hard, it would have to be in the generated code, not in the generator).

> - CodegenLIR.cp:2169 do we even support/test
>   a build without WRITE_BARRIERS?
>   Maybe the #error should be moved to include
>   other non-supported ifdefs

I agree.  I will create a patch that precedes this LIR patch, that just makes these combinations produce errors, early on, and clean up this code.

> - CodegenLIR.cpp:3817 binaryIns(LIR_lt),  mind
>   to double check this optimization, it looks a little odd,
>   although I think its carried over verbatim from MIR
>   so its probably correct.

good catch, its wrong.  for it to work the two values have to be promoted first and we need a quad-size compare.  And, it should be AVMPLUS_64BIT not AMD64.

> - CodegenLIR.cpp:3959 is _save_eip really 32b 
>   on 64b platforms?

good catch, another bug.  save_eip is indeed 32bit values (pc offsets, not actual pointers) but debugEnter expects a pointer to sintptr, not a pointer to int32_t.  Need to fix this up.

> - CodegenLIR:cpp:3983 was this a 32b bug also?
> 
> And a general comment for which I don't have a 
> good answer, but it strikes me that we now have 
> a LIR for which one is not sure what instructions
> require support on 32b and 64b platforms.  
> 
> For example, LIR_qalloc and LIR_ialloc are both
> available in 32b builds but only ialloc is used.
> Others such as LIR_i/u2q and LIR_qlo are
> even more ambiguous; should a 32bit back-end 
> support them? 

the answer depends on both the ABI and the frontend.  In principle there's nothing wrong with a 32bit jit that wants to use the 64bit int operations; however obviously the backend will have to support those ops either via lowering them during LIR generation, or by supporting them at assembly time.  Asserts will catch unsupported opcodes either way.

> Perhaps we can ifdef AVMPLUS_64BIT around
> the LIR instructions themselves to distinguish 
> what is needed for each of the builds.
> 
> Also, I didn't look at all instances but I expect that
> Assembler should not be using any of the LIR aliases;
> this would only throw more confusion to the issue.

The aliases will no doubt be a source of trouble.  we can avoid using them, the only sure fire thing to do is remove them, and rely on helper functions for generating them, and sniffer functions for testing for them.  both add overhead.  

in places where you really want the constant value of a LIR instruction, and want the alias, and dont want to be fooled (eg in case statements), maybe macros are what we want.  they decorate the usage to avoid confusion but still boil down to the appropriate constant.  eg use PTR_SIZE(LIR_ialloc,LIR_palloc) expllicitly, instead of an alias LIR_alloc.

Comment 8

[Rick Reitmaier]

Comment on attachment 357155 [details] [diff] [review]
ppc64 backend, requires lir64.patch

Marking + without thoroughly reviewing the ppc specific 
code, let me know if you'd like something more extensive.
But I'm surmising that passing the acceptance tests in
this case is clear enough indication the code is quite alright.

One observation though:
- NativePPC underrunProtect() has br value 4 for 32b and
  7 for 64b ; just checking that it shouldn't be 8?

Comment 9

[Edwin Smith]

(In reply to comment #8)
> (From update of attachment 357155 [details] [diff] [review])
> Marking + without thoroughly reviewing the ppc specific 
> code, let me know if you'd like something more extensive.
> But I'm surmising that passing the acceptance tests in
> this case is clear enough indication the code is quite alright.
> 
> One observation though:
> - NativePPC underrunProtect() has br value 4 for 32b and
>   7 for 64b ; just checking that it shouldn't be 8?

7 is correct.  the worst case branch code that constructs a 64bit immediate const to jump to, takes 7 instructions.

ppc64 (and ppc32, maybe) should make use of a constant pool and put absolute addresses in it, however since it has no PC-relative addressing, we'll need a dedicated register to point to const data near the code, and that's too much for this patch.

Comment 10

[Andreas Gal :gal]

Comment on attachment 357154 [details] [diff] [review]
64bit upgrades for LIR notation (no new backends)

Comment from david: would be nice to split LIR_float and LIR_quad too

Comment 11

[Edwin Smith]

Comment on attachment 357154 [details] [diff] [review]
64bit upgrades for LIR notation (no new backends)

pushed to tamarin-redux
changeset:   1313:de598a38ae61

Comment 12

[Edwin Smith]

Comment on attachment 357155 [details] [diff] [review]
ppc64 backend, requires lir64.patch

pushed to tamarin-redux
changeset:   1315:841135282486

Comment 13

[Brent Baker]

removing QRB request, bug resolved/verified