742606 - IonMonkey: Lowering of ArrayPopShift fails on "undefined" output (Assertion failure: unexpected type, at./js/src/ion/LIR.h:568)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

The following test case asserts with --ion -n and also with --ion-eager.

var a = new Array();
for (i = 0; i < 50; i++)
  a.shift();

The problem come from the excepted known type of the operation which is not consistent with the LIR representation which excepts one output register.  The convertion of the known type to the MIR is part of the hypothesis and should be move before the discardCall function.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Do not generate MIR for undefined, null returned values.

This patch should fix the bug, but it does not attempt to optimize this case of undefined or null values returned by Array.shift.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Do not generate MIR for undefined, null returned values.

Add type barrier after MArrayPopShift as the result can change.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 612713 [details] [diff] [review]
Do not generate MIR for undefined, null returned values.

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

::: js/src/ion/MCallOptimize.cpp
@@ +123,5 @@
>                  bool needsHoleCheck = thisTypes->hasObjectFlags(cx, types::OBJECT_FLAG_NON_PACKED_ARRAY);
>                  bool maybeUndefined = returnTypes->hasType(types::Type::UndefinedType());
>                  MArrayPopShift::Mode mode = (native == js::array_pop) ? MArrayPopShift::Pop : MArrayPopShift::Shift;
>                  JSValueType knownType = returnTypes->getKnownTypeTag(cx);
> +                if (knownType == JSVAL_TYPE_UNDEFINED && knownType == JSVAL_TYPE_NULL)

oops, with an || instead of &&.  Now the test pass again.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 612713 [details] [diff] [review]
Do not generate MIR for undefined, null returned values.

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

r=me without the barrier.

::: js/src/ion/MCallOptimize.cpp
@@ +124,5 @@
>                  bool maybeUndefined = returnTypes->hasType(types::Type::UndefinedType());
>                  MArrayPopShift::Mode mode = (native == js::array_pop) ? MArrayPopShift::Pop : MArrayPopShift::Shift;
>                  JSValueType knownType = returnTypes->getKnownTypeTag(cx);
> +                if (knownType == JSVAL_TYPE_UNDEFINED && knownType == JSVAL_TYPE_NULL)
> +                    return false;

Most of the other instructions use MIRType_Value when the type is null/undefined, but in this case it's rare so not inlining is fine.

@@ +132,5 @@
>                  MArrayPopShift *ins = MArrayPopShift::New(argv[0], mode, needsHoleCheck, maybeUndefined);
>                  current->add(ins);
>                  current->push(ins);
> +                ins->setResultType(resultType);
> +                return resumeAfter(ins) && pushTypeBarrier(ins, returnTypes, barrier);

I don't think we need a barrier here (and JM doesn't use one either).

Comment 6

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/a4c89230f5ba