The default bug view has changed. See this FAQ.

IonMonkey: Compile Add with String and Objects (Assertion failure: NYI, at js/src/ion/Lowering.cpp:381)

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
…/js --ion -n …/jit-test/tests/ion/bug691747.js
Assertion failure: NYI, at /home/nicolas/mozilla/ionmonkey/js/src/ion/Lowering.cpp:381
(Assignee)

Updated

5 years ago
Blocks: 677337
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 715276
(Assignee)

Updated

5 years ago
Depends on: 714428
(Assignee)

Comment 2

5 years ago
Created attachment 586330 [details] [diff] [review]
Implement MConcat and MAddGeneric.

Fix:
- Argument order of the interpreter.
- *AtStart LUse.
- (AdjustInput) Unbox only if boxed.
Attachment #585088 - Attachment is obsolete: true
Attachment #586330 - Flags: review?(dvander)
(Assignee)

Comment 3

5 years ago
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 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.
Attachment #586330 - Flags: review?(dvander) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 587165 [details] [diff] [review]
Fix recovery of pc & script for inlined functions.
Attachment #587165 - Flags: review?(dvander)
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 **
Attachment #587165 - Flags: review?(dvander) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/41a9e0d849ea
https://hg.mozilla.org/projects/ionmonkey/rev/ee506186bc06
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.