IonMonkey: minor assembler improvements/fixes

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
Attachment #601572 - Flags: review?(Jacob.Bramley)
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?
Attachment #601572 - Flags: review?(Jacob.Bramley) → review+
(Reporter)

Comment 2

5 years ago
landed: http://hg.mozilla.org/projects/ionmonkey/rev/46fe3e36c11b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 996883
You need to log in before you can comment on or make changes to this bug.