Closed Bug 742606 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
This patch should fix the bug, but it does not attempt to optimize this case of undefined or null values returned by Array.shift.
Add type barrier after MArrayPopShift as the result can change.
Attachment #612430 - Attachment is obsolete: true
Attachment #612713 - Flags: review?(jdemooij)
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 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).
Attachment #612713 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/a4c89230f5ba
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.