IonMonkey: Compile JSOP_POS

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 579296 [details] [diff] [review]
Implemented JSOP_POS

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+

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
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.
Attachment #579296 - Attachment is obsolete: true
Attachment #581656 - Flags: review?(jandemooij)

Updated

6 years ago
Attachment #581656 - Flags: review?(jandemooij) → review+
Pushed: 

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

Thanks again for taking this.
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.