Closed Bug 721280 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(3 files)

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.
Assignee: general → dvander
Status: NEW → ASSIGNED
FWIW, bug 706328 handles this for the SETELEM case, which I'll rebase today.
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.
Attachment #592024 - Flags: review?(jdemooij)
ARM support
Attachment #592033 - Flags: review?(mrosenberg)
(Note, most of these patches are refactoring to use the new ToOutValue.)
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.
Attachment #592024 - Flags: review?(jdemooij) → review+
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.
Attachment #592033 - Flags: review?(mrosenberg) → review+
Blocks: 706328
Attachment #592030 - Flags: review?(sstangl) → review+
You need to log in before you can comment on or make changes to this bug.