Note: There are a few cases of duplicates in user autocompletion which are being worked on.

IonMonkey: implement LoadValue

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Created attachment 581894 [details] [diff] [review]
implement LoadValue

This is needed for implementing callVM.
The implementation could have been straightforward, but I decided to spice it up a bit.
Attachment #581894 - Flags: review?(Jacob.Bramley)
Comment on attachment 581894 [details] [diff] [review]
implement LoadValue

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

Look good.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +758,3 @@
>  {
> +    JS_ASSERT(rt.code() & 1 == 0);
> +    JS_ASSERT(rt2.code() == rt.code() + 1);

Do you need to use some kind of USED() macro to avoid a warning about an unused rt2 in release mode?

@@ +1295,5 @@
>      Operand payload = ToPayload(src);
>      Operand type = ToType(src);
> +    // TODO: copy this code into a generic function that acts on all sequences of memory accesses
> +    if (((val.payloadReg().code() & 1) == 0) &&
> +        val.typeReg().code() == val.payloadReg().code()+1)

It'd be nice to have some kind of utility method for this, so you could just write something like this: is_ldrd_strd_pair(val)

@@ +1303,5 @@
> +        int offset = src.disp();
> +        if (offset < 256 && offset > -256) {
> +            ma_ldrd(EDtrAddr(Register::FromCode(src.base()), EDtrOffImm(src.disp())), val.payloadReg(), val.typeReg());
> +            return;
> +        }

If the offset (magnitude) is between 256 and 4096, it's probably best to fall back to two LDRs, as you have done here. However, if it's 4096 or greater, would it not be better to do some pointer arithmetic and then an LDRD? Either that, or calculate a base address once for two LDRs. I think JM could do that. Otherwise, long-range loads generated by this function will look like this:

add     ip, rb, #(offset & ~0xfff)
ldr     payload, [ip, #(offset & 0xfff)]
add     ip, rb, #((offset+4) & ~0xfff)
ldr     type, [ip, #((offset+4) & 0xfff)]

In the worst case where the upper part of the offset doesn't fit in an Operand 2, those adds would also be preceeded by literal-pool loads.

(Not important for now. Your new code is an improvement regardless.)

@@ +1310,5 @@
> +
> +    if (val.payloadReg().code() < val.typeReg().code()) {
> +        if (src.disp() <= 4 && src.disp() >= -8 && src.disp() & 3 == 0) {
> +            // turns out each of the 4 value -8, -4, 0, 4 corresponds exactly with one of
> +            // LDM{DB, DA, IA, IB}

Clever!
Attachment #581894 - Flags: review?(Jacob.Bramley) → review+
Comment on attachment 581894 [details] [diff] [review]
implement LoadValue

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

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +758,3 @@
>  {
> +    JS_ASSERT(rt.code() & 1 == 0);
> +    JS_ASSERT(rt2.code() == rt.code() + 1);

Use DebugOnly<Register> to avoid warning in release mode.

@@ +1328,5 @@
> +                break;
> +              default:
> +                JS_NOT_REACHED("Bogus Offset for LoadValue as DTM");
> +            }
> +            startDataTransferM(IsLoad, Register::FromCode(src.base()), mode);

The base register is modified after the load ?  I think this deserve a comment on the function prototype.

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +550,5 @@
>      masm.ma_b(&exception,Assembler::Zero);
>  
>      // Load the outparam and free any allocated stack.
>      if (f.outParam == Type_Value) {
> +        masm.ma_ldrd(EDtrAddr(sp, EDtrOffImm(0)), JSReturnReg_Data, JSReturnReg_Type);

masm.loadValue(Operand(sp, 0), JSReturnOperand);
http://hg.mozilla.org/projects/ionmonkey/rev/5aee3edf91db
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.