Update ARM JIT for TC, support new opcodes

VERIFIED FIXED

Status

Tamarin
Baseline JIT (CodegenLIR)
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Edwin Smith, Assigned: Edwin Smith)

Tracking

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
This tracks updating nanojit for TC, and will include several patches as work progresses.
(Assignee)

Comment 1

9 years ago
see work in progress in
http://hg.mozilla.com/users/edwsmith_adobe.com/armjit
(Assignee)

Comment 2

9 years ago
Created attachment 343915 [details] [diff] [review]
make asm_output[123] be a varadic macro, easier to use, supports more args
Assignee: nobody → edwsmith
Attachment #343915 - Flags: review?(rreitmai)
(Assignee)

Updated

9 years ago
Attachment #343915 - Flags: review?(rreitmai) → review?(danderson)
(Assignee)

Updated

9 years ago
Attachment #343915 - Flags: review?(rreitmai)
Attachment #343915 - Flags: review?(danderson) → review+

Comment 3

9 years ago
Comment on attachment 343915 [details] [diff] [review]
make asm_output[123] be a varadic macro, easier to use, supports more args

looks like a good cleanup.
Attachment #343915 - Flags: review?(rreitmai) → review+
(Assignee)

Comment 4

9 years ago
Comment on attachment 343915 [details] [diff] [review]
make asm_output[123] be a varadic macro, easier to use, supports more args

first patch pushed to tamarin-redux
Attachment #343915 - Attachment is obsolete: true
(Assignee)

Comment 5

9 years ago
Created attachment 345124 [details] [diff] [review]
reduce accidental spilling, other minor tweaks
Attachment #345124 - Flags: review?(rreitmai)
(Assignee)

Comment 6

9 years ago
second patch in the lineup for ARM. This doesn't have arm specific bugs but it fixes a register allocator bug that caused too much spilling.

Comment 7

9 years ago
Comment on attachment 345124 [details] [diff] [review]
reduce accidental spilling, other minor tweaks

- I see logic moved around in findreg2() but I don't see how this fixed a reg alloc bug.

- do we need to allocate storage for the verbose buffers?
Attachment #345124 - Flags: review?(rreitmai) → review+
(Assignee)

Comment 8

9 years ago
(In reply to comment #7)
> (From update of attachment 345124 [details] [diff] [review])
> - I see logic moved around in findreg2() but I don't see how this fixed a reg
> alloc bug.

We previously had a bug where if ib was already assigned a register, but that reg was not in the allow set, then we didn't reallocate it to an allowable register.

The fix was to add the ... || !(rmask(rb) & allow) guard to the code, to make rb get allocated to an allowed register.

this fix was busted when ib already had a register that was allowed.  in that case, we remove it from the mask so ia doesn't steal it, but removing it from the mask also interacted with the fix and caused it to spill unnecessarily.

the new code fixes both problems.

> - do we need to allocate storage for the verbose buffers?

yes, but the size isn't needed in the header file.
(Assignee)

Comment 9

9 years ago
Created attachment 345296 [details] [diff] [review]
winmo + arm-linux (old abi) support, soft-float only

This is the first main drop that enables the arm jit.

Known limitations:

- does not support ARM EABI which requires double parameters to be aligned in even numbered registers.  (in progress).  Windows Mobile and older gnu-arm toolchains require arg regs to be packed even with double args.

- boidshack, s3dmorph work with -Ojit.  minimal testing has been done.

- only tested with armv5 isa.

- hardcoded to soft-float in NativeARM.h.  ideally this could detect the compiler's mode automatically, somehow.  otherwise a config switch will be needed.
Attachment #345124 - Attachment is obsolete: true
Attachment #345296 - Flags: review?(rreitmai)
(Assignee)

Updated

9 years ago
Attachment #345296 - Flags: review?(vladimir)

Updated

9 years ago
Attachment #345296 - Flags: review?(rreitmai) → review+

Comment 10

9 years ago
Comment on attachment 345296 [details] [diff] [review]
winmo + arm-linux (old abi) support, soft-float only

Some observations/comments in no particular order:
(1) SoftFloatFilter should hi() and lo() look for LIR_qjoin in the interest in optimization.

(2) LIR_callh, do we need the findRegFor() on retReg[0] ?

(3) CallInfo do we need to optimize the get_sizes() call?  Seem like a lot of work each time its invoked

(4) LIR.cpp LIR_fcall || LIR_callh converted to LIR_call.  Does it make sense to do this in the SoftFltFilter?

(5) The XOR(r,r) optimization was removed for 0 constants.  Intentional?

(6)  We're using nMarkExe; is it time to convert over to tehe porting API?

(7) hint() ; LIR_param path where prefer is not set.  Should this be at least assert'd?
 
(8) BL_far() ; sure would be nice to have more comments in there.  I still don't follow what's being done.
Comment on attachment 345296 [details] [diff] [review]
winmo + arm-linux (old abi) support, soft-float only

(Only looked at nanojit pieces)

Codewise looks fine, though I think there are a few things that I'll need to compare with changes I've made in tracemonkey.  I really like the simplification of the ARM-specific code, though.

But, please please fix your editor's indentation -- most of nanojit has fairly crazy indentation, but NativeARM.{cpp,h} is clean, would be good to keep it that way :-) (no hard tabs, 4-space indents is really the main thing -- most of the new code seems fine actually, but there are some oddnesses.. they're visible in bugzilla pretty easily).
Attachment #345296 - Flags: review?(vladimir) → review+
(Assignee)

Comment 12

9 years ago
(In reply to comment #10)
> (From update of attachment 345296 [details] [diff] [review])
> Some observations/comments in no particular order:
> (1) SoftFloatFilter should hi() and lo() look for LIR_qjoin in the interest in
> optimization.

We only call lo(a) and hi(a) when a is not a qjoin.

> (2) LIR_callh, do we need the findRegFor() on retReg[0] ?

yes.  say the call is to a pure (cse-enabled function).  if callh is live, then we must make the call live too.  keeping the callh without emitting code for the call would of course be bogus.

> (3) CallInfo do we need to optimize the get_sizes() call?  Seem like a lot of
> work each time its invoked

we only called it from two places (once generating LIR, once generating asm).  The call from LirBufWriter wasn't needed, changed to count_args().

> (4) LIR.cpp LIR_fcall || LIR_callh converted to LIR_call.  Does it make sense
> to do this in the SoftFltFilter?

Its ugly the way it is now, but the filter api is insCall(CallInfo*, LIns**), the opcode is determined by LirBufWriter only.  Cleaning this up can be a future todo but i dont think we want to change the LirFilter call api.  instead maybe we can handle it partly in softfloatfilter and partly in the native assembler code.

> (5) The XOR(r,r) optimization was removed for 0 constants.  Intentional?

yes, arm convention is MOV r,#0, it's the same size instruction as xor and already handled by the case for 8bit immediates.

> (6)  We're using nMarkExe; is it time to convert over to tehe porting API?

once tamarin-redux actually uses the porting api, yes.  currently it's not being used anywhere.

> (7) hint() ; LIR_param path where prefer is not set.  Should this be at least
> assert'd?

no, if the param is >= 4, then it will go on the stack, no hinting is needed in that case.  it's not a bug requiring an assert.

> (8) BL_far() ; sure would be nice to have more comments in there.  I still
> don't follow what's being done.

i'll remove the invalid comments, but i dont see whats unclear about the remaining comments.  please elaborate (contact me directly if you want)
(Assignee)

Comment 13

9 years ago
(In reply to comment #11)
> (From update of attachment 345296 [details] [diff] [review])

> But, please please fix your editor's indentation

done
(Assignee)

Updated

9 years ago
Blocks: 465737

Comment 14

9 years ago
Created attachment 349451 [details] [diff] [review]
Build changes for MMgc/WinCEUtil.armasm
Attachment #349451 - Flags: review?(edwsmith)
(Assignee)

Comment 15

9 years ago
pushed (last friday) to tamarin-redux as changeset:   1146:c9be5727a5c0
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Attachment #349451 - Flags: review?(edwsmith) → review+

Comment 16

8 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.