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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 612430 [details] [diff] [review]
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.
Created attachment 612713 [details] [diff] [review]
Do not generate MIR for undefined, null returned values.

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.
Duplicate of this bug: 743077
Blocks: 349611
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.