IonMonkey: Compile JSOP_PICK & JSOP_SWAP

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 584351 [details] [diff] [review]
Implement JSOP_PICK & JSOP_SWAP.
Attachment #584351 - Flags: review?(dvander)
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.)
Attachment #584351 - Flags: review?(dvander)
(Assignee)

Comment 3

6 years ago
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.
Attachment #584351 - Attachment is obsolete: true
Attachment #585593 - Flags: review?(dvander)
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.
Attachment #585593 - Flags: review?(dvander) → review+
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/2ed7cba609d7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.