721280 - IonMonkey: Don't bailout on all out-of-bounds array accesses

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

ai-astar's neighbor() function tests out-of-bounds array accesses sort of frequently, using index -1. We currently bailout every time. We should stay in JIT for this case, like JM+TI.

Comment 1

[Jan de Mooij [:jandem]]

FWIW, bug 706328 handles this for the SETELEM case, which I'll rebase today.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1: x86 version

I think useRegisterAtStart() should be possible for at least one of the parameters, but doing so for the initLength gives me errors in crypto-sha1. I see stuff like:

      277 loadelementhole ([t:277 (=ecx)], [d:278 (=eax)]) (=esi), (=edi), (=edi) <|@

So it seems like two different inputs are getting the same physical register. So for now I just did useRegister.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2: x64 support

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3: ARM support

ARM support

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

(Note, most of these patches are refactoring to use the new ToOutValue.)

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 592024 [details] [diff] [review]
part 1: x86 version

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

Nice work, the BaseIndex and ToOutValue abstractions are good to avoid platform-specific code.

::: js/src/ion/CodeGenerator.cpp
@@ +1202,5 @@
>      return callVM(ThrowInfo, lir);
>  }
>  
> +bool
> +CodeGenerator::visitLoadElementHole(LLoadElementHole *lir)

Really nice and concise. Would you mind rewriting LoadElementV like this?

@@ +1213,5 @@
> +    // value.
> +    Label undefined, done;
> +    if (lir->index()->isConstant()) {
> +        masm.branch32(Assembler::BelowOrEqual, initLength, Imm32(ToInt32(lir->index())), &undefined);
> +        masm.loadValue(Address(elements, ToInt32(lir->index())), out);

Shouldn't this be ToInt32(lir->index()) * sizeof(js::Value)?

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +536,5 @@
>          ma_cmp(ScratchRegister, imm);
>          ma_b(label, InvertCondition(cond));
>      }
>      void branchPtr(Condition cond, Register lhs, Register rhs, Label *label) {
> +        return branch32(cond, lhs, rhs, label);

Nit: remove the "return"?

@@ +543,5 @@
>          movePtr(ptr, ScratchRegister);
>          branchPtr(cond, lhs, ScratchRegister, label);
>      }
>      void branchPtr(Condition cond, Register lhs, ImmWord imm, Label *label) {
> +        return branch32(cond, lhs, Imm32(imm.value), label);

Ditto.

Comment 7

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 592033 [details] [diff] [review]
part 3: ARM support

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

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +880,5 @@
> +    Register typeReg = ToRegister(ins->getDef(TYPE_INDEX));
> +    Register payloadReg = ToRegister(ins->getDef(PAYLOAD_INDEX));
> +    return ValueOperand(typeReg, payloadReg);
> +}
> +

Wow, that cleans things up a lot more than I would have expected.

@@ +1110,5 @@
>      Register base = ToRegister(load->elements());
> +    if (load->index()->isConstant())
> +        masm.loadValue(Address(base, ToInt32(load->index()) * sizeof(Value)), out);
> +    else
> +        masm.loadValue(base, ToRegister(load->index()), out);

Do we want to bundle base and ToRegister(load->index()) into a single BaseIndex?

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +1238,5 @@
>          : AboveOrEqual;
>      ma_cmp(value.typeReg(), ImmTag(JSVAL_TAG_CLEAR));
>      return actual;
>  }
> +

Initially, I'd left out all of the spaces because all of these functions formed a cohesive unit (at least in my head).  Without putting these in a separate class, is there some nice way to visually distinguish this block of functions as a single unit?  I suspect a divider-comment is the solution.

@@ +1539,5 @@
>  {
>      if (isValueDTRDCandidate(val)) {
>          Register tmpIdx;
> +        ma_lsl(Imm32(TimesEight), index, ScratchRegister);
> +        tmpIdx = ScratchRegister;

Since there is only one case here, tmpIdx isn't needed, hard-coding ScratchRegister on the next line should be fine.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/projects/ionmonkey/rev/fd1bdc763619
http://hg.mozilla.org/projects/ionmonkey/rev/f8ea86249dda
http://hg.mozilla.org/projects/ionmonkey/rev/0818792fd461