1172470 - Unboxed case in GenerateDenseElementHole should never be taken.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Tooru Fujisawa [:arai]]

Derived from bug 1171777.

In LIRGenerator::visitGetElementCache, the boxed/unboxed of index and output should be same.
https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Lowering.cpp#3230
>     if (ins->type() == MIRType_Value) {
>         MOZ_ASSERT(ins->index()->type() == MIRType_Value);
>         LGetElementCacheV* lir = new(alloc()) LGetElementCacheV(useRegister(ins->object()));
>         useBox(lir, LGetElementCacheV::Index, ins->index());
>         defineBox(lir, ins);
>         assignSafepoint(lir, ins);
>     } else {
>         MOZ_ASSERT(ins->index()->type() == MIRType_Int32);
>         LGetElementCacheT* lir = new(alloc()) LGetElementCacheT(useRegister(ins->object()),
>                                                                 useRegister(ins->index()));
>         define(lir, ins);
>         assignSafepoint(lir, ins);
>     }

And in GetElementIC::canAttachDenseElementHole, output should be boxed.
https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonCaches.cpp#3529
>     if (!output.hasValue())
>         return false;

So, in GenerateDenseElementHole, index should always be boxed, and it doesn't have to generate a code for unboxed value.
https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonCaches.cpp#3456
>     if (index.reg().hasValue()) {

Assert that else-branch is never taken, and remove it.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Remove unused unboxed case from GenerateDenseElementHole.

Just removed else-branch and delayed assignment of Registers, and added an assertion for boxed, which passes :)

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a307c9491561

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8620796 [details] [diff] [review]
Remove unused unboxed case from GenerateDenseElementHole.

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

Beautiful, thanks!

::: js/src/jit/IonCaches.cpp
@@ +3628,5 @@
> +    // Make sure index is nonnegative.
> +    masm.branch32(Assembler::LessThan, indexReg, Imm32(0), &failures);
> +
> +    // Save the object register.
> +    masm.push(object);

Nit: you can move the |Register elementsReg = object;| right before this line, that makes it a little more clear we're pushing it because it'll be used as elementsReg.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3045335448

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba3045335448