Last Comment Bug 742606 - IonMonkey: Lowering of ArrayPopShift fails on "undefined" output (Assertion failure: unexpected type, at./js/src/ion/LIR.h:568)
: IonMonkey: Lowering of ArrayPopShift fails on "undefined" output (Assertion f...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
: 743077 (view as bug list)
Depends on:
Blocks: jsfunfuzz IonEager
  Show dependency treegraph
 
Reported: 2012-04-04 18:42 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-04-09 14:18 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Do not generate MIR for undefined, null returned values. (1.95 KB, patch)
2012-04-04 18:49 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Do not generate MIR for undefined, null returned values. (2.88 KB, patch)
2012-04-05 15:37 PDT, Nicolas B. Pierron [:nbp]
jdemooij: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-04-04 18:42:31 PDT
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] 2012-04-04 18:49:11 PDT
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.
Comment 2 Nicolas B. Pierron [:nbp] 2012-04-05 15:37:25 PDT
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.
Comment 3 Nicolas B. Pierron [:nbp] 2012-04-05 16:13:58 PDT
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 4 Nicolas B. Pierron [:nbp] 2012-04-06 17:21:07 PDT
*** Bug 743077 has been marked as a duplicate of this bug. ***
Comment 5 Jan de Mooij [:jandem] 2012-04-09 04:30:22 PDT
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] 2012-04-09 14:18:00 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/a4c89230f5ba

Note You need to log in before you can comment on or make changes to this bug.