Last Comment Bug 460764 - Update ARM JIT for TC, support new opcodes
: Update ARM JIT for TC, support new opcodes
Status: VERIFIED FIXED
:
Product: Tamarin
Classification: Components
Component: Baseline JIT (CodegenLIR) (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Edwin Smith
:
Mentors:
Depends on:
Blocks: 465737
  Show dependency treegraph
 
Reported: 2008-10-20 08:21 PDT by Edwin Smith
Modified: 2009-10-13 16:48 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
make asm_output[123] be a varadic macro, easier to use, supports more args (107.96 KB, patch)
2008-10-20 08:28 PDT, Edwin Smith
dvander: review+
rreitmai: review+
Details | Diff | Splinter Review
reduce accidental spilling, other minor tweaks (10.55 KB, patch)
2008-10-28 11:35 PDT, Edwin Smith
rreitmai: review+
Details | Diff | Splinter Review
winmo + arm-linux (old abi) support, soft-float only (105.66 KB, patch)
2008-10-29 08:51 PDT, Edwin Smith
rreitmai: review+
vladimir: review+
Details | Diff | Splinter Review
Build changes for MMgc/WinCEUtil.armasm (2.75 KB, patch)
2008-11-21 10:45 PST, Rob Winchell
edwsmith: review+
Details | Diff | Splinter Review

Description Edwin Smith 2008-10-20 08:21:56 PDT
This tracks updating nanojit for TC, and will include several patches as work progresses.
Comment 1 Edwin Smith 2008-10-20 08:23:02 PDT
see work in progress in
http://hg.mozilla.com/users/edwsmith_adobe.com/armjit
Comment 2 Edwin Smith 2008-10-20 08:28:54 PDT
Created attachment 343915 [details] [diff] [review]
make asm_output[123] be a varadic macro, easier to use, supports more args
Comment 3 Rick Reitmaier 2008-10-21 11:50:26 PDT
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.
Comment 4 Edwin Smith 2008-10-22 08:42:44 PDT
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
Comment 5 Edwin Smith 2008-10-28 11:35:01 PDT
Created attachment 345124 [details] [diff] [review]
reduce accidental spilling, other minor tweaks
Comment 6 Edwin Smith 2008-10-28 11:36:34 PDT
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 Rick Reitmaier 2008-10-28 12:02:00 PDT
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?
Comment 8 Edwin Smith 2008-10-28 12:07:59 PDT
(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.
Comment 9 Edwin Smith 2008-10-29 08:51:18 PDT
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.
Comment 10 Rick Reitmaier 2008-10-29 17:11:23 PDT
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 11 Vladimir Vukicevic [:vlad] [:vladv] 2008-10-29 17:58:38 PDT
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).
Comment 12 Edwin Smith 2008-10-30 09:22:43 PDT
(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)
Comment 13 Edwin Smith 2008-10-30 09:23:22 PDT
(In reply to comment #11)
> (From update of attachment 345296 [details] [diff] [review])

> But, please please fix your editor's indentation

done
Comment 14 Rob Winchell 2008-11-21 10:45:51 PST
Created attachment 349451 [details] [diff] [review]
Build changes for MMgc/WinCEUtil.armasm
Comment 15 Edwin Smith 2008-11-24 06:55:27 PST
pushed (last friday) to tamarin-redux as changeset:   1146:c9be5727a5c0
Comment 16 Chris Peyer 2009-10-13 16:48:19 PDT
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.

Note You need to log in before you can comment on or make changes to this bug.