Last Comment Bug 701965 - IonMonkey: Compile JSOP_ITER
: IonMonkey: Compile JSOP_ITER
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on: 713754
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-11-11 18:00 PST by Nicolas B. Pierron [:nbp]
Modified: 2012-02-08 02:15 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (edde637d2661) (23.19 KB, patch)
2011-12-27 12:59 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
Patch v1 (23.30 KB, patch)
2012-02-07 10:17 PST, Jan de Mooij [:jandem] (PTO until July 31)
no flags Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-11-11 18:00:30 PST
Necessary for benchmarks.
Comment 1 Brian Hackett (:bhackett) 2011-12-27 12:59:29 PST
Created attachment 584471 [details] [diff] [review]
WIP (edde637d2661)

Stub calls for the iterator opcodes, works on x86.  Needs fast paths for native iterators.
Comment 2 Nicolas B. Pierron [:nbp] 2011-12-27 18:11:40 PST
Comment on attachment 584471 [details] [diff] [review]
WIP (edde637d2661)

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

::: js/src/ion/CodeGenerator.cpp
@@ +687,5 @@
> +bool
> +CodeGenerator::visitCallIteratorStart(LCallIteratorStart *lir)
> +{
> +    static const VMFunction info(
> +        JS_FUNC_TO_DATA_PTR(void *, GetIteratorObject),

We no longer declare the VMFunction by hand, use templates here with the redefinition of the arguments in a typedef.  See NewArray and InvokeFunction to have examples of usage.

@@ +705,5 @@
> +bool
> +CodeGenerator::visitCallIteratorNext(LCallIteratorNext *lir)
> +{
> +    static const VMFunction info(
> +        JS_FUNC_TO_DATA_PTR(void *, js_IteratorNext),

idem.

@@ +721,5 @@
> +bool
> +CodeGenerator::visitCallIteratorMore(LCallIteratorMore *lir)
> +{
> +    static const VMFunction info(
> +        JS_FUNC_TO_DATA_PTR(void *, js_IteratorMore),

idem.

@@ +744,5 @@
> +bool
> +CodeGenerator::visitCallIteratorEnd(LCallIteratorEnd *lir)
> +{
> +    static const VMFunction info(
> +        JS_FUNC_TO_DATA_PTR(void *, js_CloseIterator),

idem.

::: js/src/ion/LIR-Common.h
@@ +1159,5 @@
> +        return mir_->toIteratorMore();
> +    }
> +};
> +
> +class LCallIteratorEnd : public LVMCallInstructionHelper<LDefinition::INTEGER, 0, 1, 0>

This is wrong, the value returned by the function must correspond to the number of definition.  Failing to respect this rule implies the production of corrupted snapshot/safepoint generated for the call.

The call is the last instruction generated.

::: js/src/ion/LIR.h
@@ +818,5 @@
>  
>    private:
>      static uint32 defMask() {
> +        if (Defs == 0)
> +            return 0;

This is wrong, because the call must be the last instruction in the CodeGenerator visit function.

::: js/src/ion/Lowering.cpp
@@ +858,5 @@
> +bool
> +LIRGenerator::visitIteratorMore(MIteratorMore *ins)
> +{
> +    LCallIteratorMore *lir = new LCallIteratorMore(useRegister(ins->iterator()));
> +    return defineVMReturn(lir, ins) && assignSafepoint(lir, ins);

Nice, usually we do the opposite order, I don't think there is one.  But I am not sure we want to change the writting style for this file …

if (!assignSafepoint(lir, ins))
    return false;
return defineVMReturn(lir, ins);

dvander ?

::: js/src/ion/MIRGraph.cpp
@@ +143,5 @@
>  
>  void
>  MBasicBlock::copySlots(MBasicBlock *from)
>  {
> +    JS_ASSERT(stackPosition_ == from->stackPosition_);

I am not sure to understand the impliciation of the modification made in this file … anyone else ?
Comment 3 Jan de Mooij [:jandem] (PTO until July 31) 2012-02-05 23:49:00 PST
We need this for ss-fasta. Brian, since you're working on other things atm, I can also rebase and land this for you, if you want.
Comment 4 Brian Hackett (:bhackett) 2012-02-06 07:37:58 PST
(In reply to Jan de Mooij (:jandem) from comment #3)
> We need this for ss-fasta. Brian, since you're working on other things atm,
> I can also rebase and land this for you, if you want.

Sure, go ahead.  This just has slow paths for iteration stuff, but with bug 713754 landed it should not be hard to add fast paths.
Comment 5 Jan de Mooij [:jandem] (PTO until July 31) 2012-02-07 10:17:35 PST
Created attachment 595077 [details] [diff] [review]
Patch v1

Rebased, will land this tomorrow.
Comment 6 Jan de Mooij [:jandem] (PTO until July 31) 2012-02-08 02:15:02 PST
Pushed (with bhackett as author):

http://hg.mozilla.org/projects/ionmonkey/rev/e4fb2cc5006a

Filed bug 725241 for the fast paths.

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