Last Comment Bug 701956 - IonMonkey: Compile JSOP_DUP2
: IonMonkey: Compile JSOP_DUP2
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-11-11 17:51 PST by Nicolas B. Pierron [:nbp]
Modified: 2011-11-29 02:53 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Compile dup and dup2 (2.64 KB, patch)
2011-11-25 07:59 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch v2 (4.02 KB, patch)
2011-11-28 03:41 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch v2 (4.02 KB, patch)
2011-11-28 03:56 PST, Jan de Mooij [:jandem]
sstangl: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-11-11 17:51:56 PST
Necessary for benchmarks.
Comment 1 Jan de Mooij [:jandem] 2011-11-25 07:59:02 PST
Created attachment 576939 [details] [diff] [review]
Compile dup and dup2

dup2 is the last op we need for bitops-nsieve-bits (on top of getelem/setelem, and after working around missing OSR).
Comment 2 Sean Stangl [:sstangl] 2011-11-26 00:48:25 PST
Comment on attachment 576939 [details] [diff] [review]
Compile dup and dup2

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

::: js/src/ion/IonBuilder.cpp
@@ +680,5 @@
>        case JSOP_SETELEM:
>          return jsop_setelem();
>  
> +      case JSOP_DUP:
> +        current->push(current->peek(-1));

Duplication must ensure that StackSlot::copyOf is correctly set: since JSOP_DUP copies a value on the stack, we actually want a function that appends to the linked list of copies.

pushCopy(uint32 slot) has a prototype in MIRGraph.h but no implementation -- maybe a relic. Would you mind removing it as part of this patch?

pushVariable(uint32 slot) looks like what we want.

@@ +1689,5 @@
> +{
> +    MDefinition *lhs = current->peek(-2);
> +    MDefinition *rhs = current->peek(-1);
> +    current->push(lhs);
> +    current->push(rhs);

Needs the same change to respect copies.
Comment 3 Jan de Mooij [:jandem] 2011-11-28 03:41:25 PST
Created attachment 577214 [details] [diff] [review]
Patch v2

Thanks. This patch uses pushSlot, since pushVariable is private (and pushSlot calls pushVariable).
Comment 4 Jan de Mooij [:jandem] 2011-11-28 03:56:53 PST
Created attachment 577216 [details] [diff] [review]
Patch v2

Make the assert a bit stronger. Sorry for the bugspam.
Comment 5 Jan de Mooij [:jandem] 2011-11-29 02:53:21 PST
Pushed:

http://hg.mozilla.org/projects/ionmonkey/rev/7665e358c6a2

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