1028675 - IonMonkey: Implement StringSplit Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[David Moreira [:vash]]

Implement the StringSplit.
See Bug 1003801 comment 0 for explanation.

Comment 1

[David Moreira [:vash]]

Attachment: wip patch

patch doesn't work (but doesn't break the test). The function StringSplit seems to be inlined (js::jit::LIRGenerator::visitStringSplit return true and js::jit::Compile return Method_Compiled), but isn't called.

I think that the prototype of the function isn't correct. Should we stay with a function of arity two?

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8449010 [details] [diff] [review]
wip patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +5442,2 @@
>  
>      return callVM(StringSplitInfo, lir);

Questions / Remarks:
 - Why did you invert the order of arguments? (push the outermost first)
 - We do not want to have allocations for the type objects, as this implies that we would load a constant before pushing it to the stack. (see [1])

I would be surprised if this code pass the test suite.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#3681

::: js/src/jit/LIR-Common.h
@@ +2925,5 @@
>      LIR_HEADER(StringSplit)
>  
> +    LStringSplit(const LAllocation &typeObject, const LAllocation &string,
> +                 const LAllocation &separator) {
> +        setOperand(0, typeObject);

Do not add allocations for the template Object, which is not the typeObject.

::: js/src/jit/MIR.h
@@ +4840,1 @@
>      public MixPolicy<StringPolicy<0>, StringPolicy<1> >

The reason why the function is never entered is simple.  This MixPolicy add guards before any string split MIR to check that the first and second operands are strings.  As the StringSplit instruction returns an array, the template object is will always fail the MStringGuard check added by Apply Type phase.

This explains why the function is never called.

I think the easiest would be to just make the template object be the last argument of MStringSplit and do not care about the type policy of it.

@@ +4840,3 @@
>      public MixPolicy<StringPolicy<0>, StringPolicy<1> >
>  {
> +    MStringSplit(types::CompilerConstraintList *constraints, MDefinition *typeObject,

s/MDefinition *typeObject/MConstant *templateObject/

@@ +4856,2 @@
>      }
> +    MDefinition *typeObject() const {

same thing here.
And add a getter to return the ->type() of the templateObject, and use it in the CodeGenerator visit function.

::: js/src/jit/Recover.cpp
@@ +739,5 @@
> +RStringSplit::recover(JSContext *cx, SnapshotIterator &iter) const
> +{
> +    RootedObject templateObject(cx, &iter.read().toObject());
> +    RootedTypeObject rooted(cx, templateObject->type());
> +    HandleTypeObject typeObject(&rooted);

There is no need to explicit the Handle construction out of the rooted.  These constructors are implicit and are made when we call the str_split_string function.

Comment 3

[David Moreira [:vash]]

Attachment: patch-v2.diff

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8451140 [details] [diff] [review]
patch-v2.diff

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

This looks good, still some small issues to get the code clean and landable ;)

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +305,5 @@
>      return i;
>  }
>  
> +var uceFault_str_split = eval(uneval(uceFault).replace('uceFault', 'uceFault_str_split'))
> +function rstr_split(i) {

follow the order of Bug 1003801 comment 0.

@@ +469,1 @@
>      rround_double(i);

same here.

::: js/src/jit/MCallOptimize.cpp
@@ +1158,5 @@
>          return InliningStatus_NotInlined;
>      }
>  
>      callInfo.setImplicitlyUsedUnchecked();
> +    MConstant *typeObject = MConstant::NewConstraintlessObject(alloc(), templateObject);

Use MConstant::New instead, and replace the call of MakeSingletonTypeSet(…) by resultTypeSet() from MStringSplit constructor.

@@ +1159,5 @@
>      }
>  
>      callInfo.setImplicitlyUsedUnchecked();
> +    MConstant *typeObject = MConstant::NewConstraintlessObject(alloc(), templateObject);
> +    current->add(typeObject);

s/typeObject/templateObjectDef/

::: js/src/jit/MIR.h
@@ +4842,3 @@
>      MStringSplit(types::CompilerConstraintList *constraints, MDefinition *string, MDefinition *sep,
> +                 MConstant *templateObj)
> +      : MTernaryInstruction(string, sep, templateObj)

s/templateObj/templateObject/

::: js/src/jit/Recover.h
@@ +374,5 @@
>  };
>  
> +class RStringSplit MOZ_FINAL : public RInstruction
> +{
> + public:

style-nit: 2 spaces before public.

Comment 5

[David Moreira [:vash]]

Attachment: patch-v3.diff

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8453261 [details] [diff] [review]
patch-v3.diff

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

One last thing, and I'll r+ this patch ;)

::: js/src/jit/MIR.h
@@ +4845,3 @@
>      {
>          setResultType(MIRType_Object);
> +        setResultTypeSet(MakeSingletonTypeSet(constraints, this->templateObject()));

as MakeSingletonTypeSet is done by MConstant::New, you just need to copy the type set of the templateObject instead of making another singleton type set.

Comment 7

[David Moreira [:vash]]

Attachment: patch-v4.diff

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8453337 [details] [diff] [review]
patch-v4.diff

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

This sounds good, I will send it to the Try server.

Comment 9

[Nicolas B. Pierron [:nbp]]

https://tbpl.mozilla.org/?tree=Try&rev=e96ac5a55f63

Comment 10

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f27562e4159

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f27562e4159