Last Comment Bug 713571 - IonMonkey: Compile JSOP_PICK & JSOP_SWAP
: IonMonkey: Compile JSOP_PICK & JSOP_SWAP
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:
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-12-26 14:51 PST by Nicolas B. Pierron [:nbp]
Modified: 2012-01-10 15:45 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement JSOP_PICK & JSOP_SWAP. (4.41 KB, patch)
2011-12-26 15:00 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Implement JSOP_PICK & JSOP_SWAP. (6.03 KB, patch)
2012-01-03 15:58 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-12-26 14:51:55 PST
We already have some compilation failures caused only by JSOP_PICK & JSOP_SWAP.  JSOP_SWAP.

Both need to manipulate the StackSlot of MBasicBloc while taking care to reorganized the linked list embedded in the extra fields of the StackSlot.
Comment 1 Nicolas B. Pierron [:nbp] 2011-12-26 15:00:17 PST
Created attachment 584351 [details] [diff] [review]
Implement JSOP_PICK & JSOP_SWAP.
Comment 2 David Anderson [:dvander] 2012-01-03 12:07:52 PST
Comment on attachment 584351 [details] [diff] [review]
Implement JSOP_PICK & JSOP_SWAP.

I'd prefer if we could solve this using existing stack transition methods, since this update code looks fairly complicated and in the past has been error-prone.

SWAP is easy: DUP2, store top to -3, pop, store top to -1, pop
PICK can be done with similar motions - see methodjit/Compiler.cpp (FrameState is the spiritual predecessor to MBasicBlock.)
Comment 3 Nicolas B. Pierron [:nbp] 2012-01-03 15:58:40 PST
Created attachment 585593 [details] [diff] [review]
Implement JSOP_PICK & JSOP_SWAP.

Simplify jsop_pick by adding swapAt function, which care only about 2 elements at
a specified depth.
Comment 4 David Anderson [:dvander] 2012-01-10 15:22:40 PST
Comment on attachment 585593 [details] [diff] [review]
Implement JSOP_PICK & JSOP_SWAP.

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

::: js/src/ion/MIRGraph.cpp
@@ +452,5 @@
> +    JS_ASSERT(lhsDepth >= info_.firstStackSlot());
> +
> +    StackSlot &lhs = slots_[lhsDepth];
> +    StackSlot &rhs = slots_[rhsDepth];
> +    StackSlot tmp;

This |tmp| declaration should be moved down to its first use,

@@ +467,5 @@
> +    updateIndexes(lhs, lhsDepth, rhsDepth);
> +    updateIndexes(rhs, rhsDepth, lhsDepth);
> +
> +    // Swap values.
> +    tmp = lhs;

... here.
Comment 5 Nicolas B. Pierron [:nbp] 2012-01-10 15:45:13 PST
https://hg.mozilla.org/projects/ionmonkey/rev/2ed7cba609d7

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