Closed Bug 707926 Opened 13 years ago Closed 12 years ago

IonMonkey: Compile JSOP_POS

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Attached patch Implemented JSOP_POS (obsolete) — Splinter Review
Here's my first proposal. Running jstests.py with --ion and --ion-eager flags show no regressions relative to the parent commit.
Attachment #579296 - Flags: review?(jdemooij)
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).
Attachment #579296 - Flags: review?(jandemooij) → review+
Status: NEW → ASSIGNED
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.
Attachment #579296 - Attachment is obsolete: true
Attachment #581656 - Flags: review?(jandemooij)
Attachment #581656 - Flags: review?(jandemooij) → review+
Pushed: 

http://hg.mozilla.org/projects/ionmonkey/rev/885782dc302a

Thanks again for taking this.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.