Last Comment Bug 708063 - IonMonkey: Increase the range of vldr and vstr
: IonMonkey: Increase the range of vldr and vstr
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
:
Mentors:
Depends on:
Blocks: 701993
  Show dependency treegraph
 
Reported: 2011-12-06 13:01 PST by Marty Rosenberg [:mjrosenb]
Modified: 2011-12-07 17:35 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Miscellaneous fixes! (14.04 KB, patch)
2011-12-06 13:01 PST, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
sstangl: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2011-12-06 13:01:13 PST
Created attachment 579424 [details] [diff] [review]
Miscellaneous fixes!

There are some other miscellaneous fixes + new features in here.
Notably, making OSR *not* bail out randomly
Comment 1 Sean Stangl [:sstangl] 2011-12-06 15:31:13 PST
Comment on attachment 579424 [details] [diff] [review]
Miscellaneous fixes!

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

Leaving the ARM changes to jbramley.

::: js/src/jsinterp.cpp
@@ +1965,5 @@
>          ion::MethodStatus status =
>              ion::CanEnterAtBranch(cx, script, regs.fp(), regs.pc);
>          if (status == ion::Method_Compiled) {
> +            interpReturnOK = ion::SideCannon(cx, regs.fp(), regs.pc);
> +            if (interpReturnOK)

Good catch.
Comment 2 Jacob Bramley [:jbramley] 2011-12-07 02:56:18 PST
Comment on attachment 579424 [details] [diff] [review]
Miscellaneous fixes!

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

::: js/src/ion/arm/Assembler-arm.cpp
@@ +529,5 @@
> +        DeferredData *deferred = data_[i];
> +        //Bind(code, deferred->label(), data + deferred->offset());
> +        deferred->copy(code, data + deferred->offset());
> +    }
> +

As discussed, change the comment to indiciate that this _is_ necessary, for switch statements.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +926,5 @@
> +MacroAssemblerARM::ma_vdtr(LoadStore ls, Operand &addr, FloatRegister rt, Condition cc)
> +{
> +    int off = addr.disp();
> +    if ((off & 3) != 0)
> +        JS_NOT_REACHED("non-multiple of 4 offsets not implemented.");

Why not just JS_ASSERT((off & 3) == 0) ?

@@ +939,5 @@
> +    int neg_bottom = (0x100 << 2) - bottom;
> +    // at this point, both off - bottom and off + neg_bottom will be reasonable-ish
> +    // quantities.
> +    if (off < 0) {
> +        Operand2 sub_off = Imm8(-(off-bottom)); // sub_off = bottom - off

This looks like it will break if off is 0x80000000. (It turns up in fuzz tests sometimes.)

@@ +1327,1 @@
>          ma_str(ScratchRegister, ToPayload(dest));

Reduce the indentation.

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