Last Comment Bug 707926 - IonMonkey: Compile JSOP_POS
: IonMonkey: Compile JSOP_POS
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: ---
Assigned To: Eddy Bruel [:ejpbruel]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 695770
  Show dependency treegraph
Reported: 2011-12-06 07:17 PST by Eddy Bruel [:ejpbruel]
Modified: 2011-12-16 06:54 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Implemented JSOP_POS (2.25 KB, patch)
2011-12-06 07:31 PST, Eddy Bruel [:ejpbruel]
jdemooij: review+
Details | Diff | Splinter Review
Patch to be commited (3.31 KB, patch)
2011-12-14 08:52 PST, Eddy Bruel [:ejpbruel]
jdemooij: review+
Details | Diff | Splinter Review

Description User image Eddy Bruel [:ejpbruel] 2011-12-06 07:17:14 PST

Comment 1 User image Eddy Bruel [:ejpbruel] 2011-12-06 07:31:00 PST
Created attachment 579296 [details] [diff] [review]
Implemented JSOP_POS

Here's my first proposal. Running with --ion and --ion-eager flags show no regressions relative to the parent commit.
Comment 2 User image Jan de Mooij [:jandem] 2011-12-07 04:35:28 PST
Comment on attachment 579296 [details] [diff] [review]
Implemented JSOP_POS

Review of attachment 579296 [details] [diff] [review]:

Looks good, thanks for taking this. You should add some tests to jit-tests/tests/ion/ though - something like testSubtract.js.

::: js/src/ion/IonBuilder.cpp
@@ +1955,5 @@
> +{
> +    TypeOracle::Unary types = oracle->unaryOp(script, pc);
> +    MDefinition *value = current->pop();
> +    MInstruction *ins;
> +    if (types.ival == MIRType_Int32)

This has to be types.rval, I think. If the argument is a string like "123", ival is MIRType_String and rval is MIRType_Int32 so we want to use ToInt32. If at some point the string becomes "123.4", MToInt32 will bailout (since 123.4 does not fit in an int), |double| will be added to the result typeset of this JSOP_POS and the next time we compile the function types.rval is MIRType_Double and we will choose ToDouble. This may not work yet though because we need on-stack invalidation (bug 686927).
Comment 3 User image Eddy Bruel [:ejpbruel] 2011-12-14 08:52:47 PST
Created attachment 581656 [details] [diff] [review]
Patch to be commited

I've made the changes requested by jandem, but since I don't have L2 commit rights yet I cannot push the patch myself. Instead, I'm attaching it here. jandem: if you would be so kind to commit this patch to the trunk, I think we can consider this bug resolved.
Comment 4 User image Jan de Mooij [:jandem] 2011-12-16 06:54:51 PST

Thanks again for taking this.

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