Last Comment Bug 713693 - IonMonkey: Compile Add with String and Objects (Assertion failure: NYI, at js/src/ion/Lowering.cpp:381)
: IonMonkey: Compile Add with String and Objects (Assertion failure: NYI, at js...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
:
Mentors:
Depends on: 714428 715276
Blocks: 684381 677337
  Show dependency treegraph
 
Reported: 2011-12-27 11:03 PST by Nicolas B. Pierron [:nbp]
Modified: 2012-01-12 10:36 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement MConcat and MAddGeneric. (70.70 KB, patch)
2011-12-30 17:36 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Implement MConcat and MAddGeneric. (47.54 KB, patch)
2012-01-05 19:40 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review
Fix recovery of pc & script for inlined functions. (13.71 KB, patch)
2012-01-09 14:55 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-12-27 11:03:40 PST
…/js --ion -n …/jit-test/tests/ion/bug691747.js
Assertion failure: NYI, at /home/nicolas/mozilla/ionmonkey/js/src/ion/Lowering.cpp:381
Comment 1 Nicolas B. Pierron [:nbp] 2011-12-30 17:36:41 PST
Created attachment 585088 [details] [diff] [review]
Implement MConcat and MAddGeneric.

No review asked, because the patch still need some clean-up and it need to be tested on ARM.
Comment 2 Nicolas B. Pierron [:nbp] 2012-01-05 19:40:19 PST
Created attachment 586330 [details] [diff] [review]
Implement MConcat and MAddGeneric.

Fix:
- Argument order of the interpreter.
- *AtStart LUse.
- (AdjustInput) Unbox only if boxed.
Comment 3 Nicolas B. Pierron [:nbp] 2012-01-05 19:43:39 PST
Currently the way of getting the script & pc from the frame and the snapshot is wrong because it could be an inlined frame. We need the script and the pc to monitor types.
Comment 4 David Anderson [:dvander] 2012-01-06 13:45:55 PST
Comment on attachment 586330 [details] [diff] [review]
Implement MConcat and MAddGeneric.

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

::: js/src/ion/IonBuilder.cpp
@@ +1949,5 @@
> +    return true;
> +}
> +
> +bool
> +IonBuilder::jsop_add_fastPath(MDefinition *left, MDefinition *right)

s/fastPath/specialize/

@@ +1957,5 @@
> +    if (!b.lhsTypes || !b.rhsTypes || !b.outTypes)
> +        return false;
> +
> +    if (b.outTypes->getKnownTypeTag(cx) == JSVAL_TYPE_INT32 ||
> +        b.outTypes->getKnownTypeTag(cx) == JSVAL_TYPE_DOUBLE) {

Nit: Brace goes on next line with multi-line conditonals.

@@ +1962,5 @@
> +
> +        if (b.lhsTypes->getKnownTypeTag(cx) != JSVAL_TYPE_INT32 &&
> +            b.lhsTypes->getKnownTypeTag(cx) != JSVAL_TYPE_DOUBLE &&
> +            b.lhsTypes->getKnownTypeTag(cx) != JSVAL_TYPE_UNDEFINED)
> +            return false;

These two |if|s should be braced.

::: js/src/ion/Lowering.cpp
@@ +381,5 @@
> +    if (ins->specialization() == MIRType_None) {
> +        JS_ASSERT(lhs->type() == MIRType_None);
> +        JS_NOT_REACHED("NYI: Cannot bounce to AddV because we need a resumepoint.");
> +        return false;
> +    }

Since MAdd is separate from MAddGeneric, you can just remove this whole if case, as well as the assert below, and above say: JS_ASSERT(ins->specialization() == MIRType_Double).

::: js/src/ion/MIR.h
@@ +1389,5 @@
> +
> +    virtual AliasSet getAliasSet() const {
> +        if (input()->type() >= MIRType_Object)
> +            return AliasSet::Store(AliasSet::Any);
> +        return AliasSet::None();

This instruction should never have an object as an input, it looks like - so it should be okay to just return AliasSet::None().

@@ +1773,5 @@
> +        return AliasSet::Store(AliasSet::Any);
> +    }
> +};
> +
> +class MBinaryStringInstruction

You can probably fold this into MConcat. I'm not immediately aware of any other binary string instructions.

::: js/src/ion/TypePolicy.h
@@ +112,5 @@
>  
> +class BinaryStringPolicy : public BoxInputsPolicy
> +{
> +  public:
> +    void specializeInputs(MInstruction *ins, TypeAnalysis *analyzer);

You will probably run into a rebase against the type analysis overhaul - luckily, you can just delete anything related to specializeInputs.

::: js/src/jsinterp.cpp
@@ +5485,5 @@
> +            res->setDouble(double(l) + double(r));
> +            TypeScript::MonitorOverflow(cx, script, pc);
> +        } else {
> +            res->setInt32(sum);
> +        }

This is probably a change we want to mirror on mozilla-central, to ease merge pain. We can do it in a follow-up bug, but it should happen. We might need to figure out how to factor script/pc first though.

::: js/src/vm/String.cpp
@@ -275,1 @@
>  js_ConcatStrings(JSContext *cx, JSString *left, JSString *right)

Heh. More jstracer vestige.
Comment 5 Nicolas B. Pierron [:nbp] 2012-01-09 14:55:02 PST
Created attachment 587165 [details] [diff] [review]
Fix recovery of pc & script for inlined functions.
Comment 6 David Anderson [:dvander] 2012-01-09 18:48:17 PST
Comment on attachment 587165 [details] [diff] [review]
Fix recovery of pc & script for inlined functions.

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

::: js/src/jsinferinlines.h
@@ +593,5 @@
>          TypeDynamicResult(cx, script, pc, Type::UnknownType());
>  }
>  
>  /* static */ inline void
> +TypeScript::GetPcScript(JSContext *cx, JSScript *&script, jsbytecode *&pc)

Nit: we usually prefer non-references for outparams, i.e. JSScript **, jsbytecode **

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