731550 - IonMonkey: minor assembler improvements/fixes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

Attachment: /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]]

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]]

landed: http://hg.mozilla.org/projects/ionmonkey/rev/46fe3e36c11b