Last Comment Bug 731550 - IonMonkey: minor assembler improvements/fixes
: IonMonkey: minor assembler improvements/fixes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on: 996883
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-29 04:19 PST by Marty Rosenberg [:mjrosenb]
Modified: 2014-04-18 10:33 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/SmallFixes-r0.patch (13.05 KB, patch)
2012-02-29 04:19 PST, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Review

Description Marty Rosenberg [:mjrosenb] 2012-02-29 04:19:55 PST
Created attachment 601572 [details] [diff] [review]
/home/mrosenberg/patches/SmallFixes-r0.patch

This is a collection of small improvements to the arm assembler and macro assembler, as well as a fix for a bug or two found in the process.
Comment 1 Jacob Bramley [:jbramley] 2012-02-29 09:10:39 PST
Comment on attachment 601572 [details] [diff] [review]
/home/mrosenberg/patches/SmallFixes-r0.patch

Review of attachment 601572 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/Ion.h
@@ +103,5 @@
>  
>      // How many invocations or loop iterations are needed before calls
>      // are inlined.
>      //
> +    // Default: 10,240

Any particular reason for the change?

::: js/src/ion/arm/Assembler-arm.cpp
@@ +685,5 @@
>              JS_ASSERT( ((no_n1 >> (32 - imm2shift)) | (no_n1 << imm2shift)) ==
>                         imm2);
>          }
> +        return TwoImm8mData(datastore::Imm8mData(imm1, imm1shift >> 1),
> +                            datastore::Imm8mData(imm2, imm2shift >> 1));

It'd be good to assert that imm<n>shift & 1 == 0.

@@ +762,5 @@
>          return op_cmp;
> +      case op_tst:
> +        JS_ASSERT(dest != InvalidReg);
> +        *imm = Imm32(~imm->value);
> +        *negDest = ScratchRegister;

This makes no sense in its current form. It looks like a partial implementation of an inverted op_tst (which doesn't exist in ARM as a single instruction). Note that this code will just fall through the switch and return op_invalid anyway.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +119,4 @@
>      Imm8 negImm8 = Imm8(negImm.value);
> +    // add r1, r2, -15 can be replaced with
> +    // sub r1, r2, 15
> +    // for bonus points, dest can be replaced (nearly always invalid => ScratchRegister)

"invalid => ScratchRegister"

Please explain (in the comment) what you mean. Is it that we can use ScratchRegister if a destination register is required by the ISA, but not specified?
Comment 2 Marty Rosenberg [:mjrosenb] 2012-03-16 16:26:44 PDT
landed: http://hg.mozilla.org/projects/ionmonkey/rev/46fe3e36c11b

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