Closed Bug 1028675 Opened 10 years ago Closed 10 years ago

IonMonkey: Implement StringSplit Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: vash, Assigned: vash, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Implement the StringSplit.
See Bug 1003801 comment 0 for explanation.
Assignee: nobody → davidmoreirafr
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Blocks: 1003801
Attached patch wip patch (obsolete) — Splinter Review
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?
Attachment #8449010 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8449010 - Flags: review?(nicolas.b.pierron)
Attached patch patch-v2.diff (obsolete) — Splinter Review
Attachment #8449010 - Attachment is obsolete: true
Attachment #8451140 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8451140 - Flags: review?(nicolas.b.pierron) → feedback+
Attached patch patch-v3.diff (obsolete) — Splinter Review
Attachment #8451140 - Attachment is obsolete: true
Attachment #8453261 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8453261 - Flags: review?(nicolas.b.pierron) → feedback+
Attached patch patch-v4.diffSplinter Review
Attachment #8453337 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8453337 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8453261 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/1f27562e4159
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.