Closed Bug 1008707 Opened 10 years ago Closed 10 years ago

IonMonkey: Implement BitOr Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: nbp, Assigned: jlevesy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=bbouvier][lang=c++])

Attachments

(1 file, 2 obsolete files)

Implement RBitOr in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
Hi Nicolas.
I'm interested in working on this bug.
I already prepared a first attempt (see attachment), based on what I've seen (and understood ^^) in bug 1003801 and 1003802. 
Hope this will help.
Attached patch First attempt without tests. (obsolete) — Splinter Review
Attachment #8421859 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8421859 [details] [diff] [review]
First attempt without tests.

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

Nice! Thanks for this contribution, it looks almost perfect. It will just need some tests before getting a r+ and landing. You can see existing tests for Add in /js/src/jit-test/tests/ion/dce-with-rinstructions.js (you can see how to build, run tests (and measure benchmark performance if you're extra brave), in [1]).

[1] https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey#Fix_something

::: js/src/jit/Recover.cpp
@@ +216,5 @@
> +    RootedValue rhs(cx, iter.read());
> +    int32_t result;
> +
> +    MOZ_ASSERT(!lhs.isObject() && !rhs.isObject());
> +

style nit: you can delete this blank line. Actually, you could even delete the assertion as there's one Ion phase that ensures that inputs to BitOr are Int32 (and thus not objects), but I'll let nbp decide whether to keep it or not.

@@ +217,5 @@
> +    int32_t result;
> +
> +    MOZ_ASSERT(!lhs.isObject() && !rhs.isObject());
> +
> +    if(!js::BitOr(cx, lhs, rhs, &result)) {

nit:
- please add a space before the opening parenthesis
- no need for curly braces as there's only one instruction in the if body.

You might want to look at the Spidermonkey coding style [1] for future reference.

[1] https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style

@@ +221,5 @@
> +    if(!js::BitOr(cx, lhs, rhs, &result)) {
> +        return false;
> +    }
> +
> +    RootedValue resultAsRoot(cx, js::Int32Value(result));

s/resultAsRoot/resultAsValue or any other better name (asValue?) you could think of :)

::: js/src/jit/Recover.h
@@ +19,5 @@
>  #define RECOVER_OPCODE_LIST(_)                  \
>      _(ResumePoint)                              \
>      _(Add)                                      \
> +    _(NewDerivedTypedObject)                    \
> +    _(BitOr)

Could you please put it before Add and just after ResumePoint, so that operations get grouped by categories, as shown in bug 1003801 comment 0)

@@ +120,5 @@
> +  public:
> +    RINSTRUCTION_HEADER_(BitOr)
> +
> +    virtual uint32_t numOperands() const {
> +        return 4;

BitOr corresponds to the | operand (for instance used in "x | 0"), so it only has two operands (which are the two values you read in the recover method).

@@ +124,5 @@
> +        return 4;
> +    }
> +
> +    bool recover(JSContext *cx, SnapshotIterator &iter) const;
> +

style nit: blank line after recover
Attachment #8421859 - Flags: review?(nicolas.b.pierron) → feedback+
Attached patch patch1008707v2.patch (obsolete) — Splinter Review
Here's an updated version, with a test case proposal for bitor on number.
I had issues with my python env, and no time at the moment to get through it, so I didn't managed at the moment to run test suite.
I only wanted to know if my test assertion ( i | ~i = -1)  seems correct to you. 
And btw, what test cases do you expect ? Object ? Any other ?
Thanks!
Attachment #8421859 - Attachment is obsolete: true
Attachment #8422229 - Flags: review?(benj)
Comment on attachment 8422229 [details] [diff] [review]
patch1008707v2.patch

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

Thanks for updating your patch. You'll need a test case for Object too (you can just adapt the test case for the BitNot operator), which should show you an error I haven't caught in the first review (sorry about that -- see comment below for the entire explanation). But one that's done, we'll be ready to land!

Could you please rebase your patch so that it applies cleanly on mozilla-inbound and add a commit message?
Let me know if you can't run the tests on your machine and I will trigger a try run with your updated patch.
Thanks!

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +48,5 @@
>  
> +var uceFault_bitor_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_bitor_number'))
> +function rbitor_number(i) {
> +    var x = i | ~i;
> +    if(uceFault_bitor_number(i)|| uceFault_bitor_number(i))

style nit: could you add a space before the if and before the || operator, for symmetry with other tests, please?

@@ +49,5 @@
> +var uceFault_bitor_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_bitor_number'))
> +function rbitor_number(i) {
> +    var x = i | ~i;
> +    if(uceFault_bitor_number(i)|| uceFault_bitor_number(i))
> +        assertEq(x, -1) /* i | ~i = -1 */

Although the test is correct, I would prefer to test only one thing at a time. Here, we're testing both the recover instruction for the BitNot (~i) and the BitOr. You can use the same idea for the test though. The assertEq will be triggered when i == 99, so you could have:

var x = i | -100; /* -100 == ~99 */
if (...)
  assertEq(x, -1); /* ~99 | 99 == -1 */

And that's the only number in the range [0, 100[ for which this assertion is true.

::: js/src/jit/MIR.h
@@ +3466,5 @@
>          return getOperand(0); // x | x => x
>      }
>      void computeRange(TempAllocator &alloc);
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    bool canRecoverOnBailout() const { return true; }

Sorry not to have caught that in the first review, but it seems this last line isn't correct and should be changed to the same thing as MBitNot.

TL;DR: look at the Object test case for BitNot (~) and you'll hit a failure when writing a similar test case.

If one of BitOr's operands is an object, it will get converted into an Int32 first, taking the default value (for Int32, this is 0) OR the value returned by the method valueOf if there's one. In the latter case, valueOf can capture a value (from the function environment, e.g. in lambdas) which changes afterwards. When reconstructing the interpreter stack, we will use the changed value to recompute the BitOr, which is not the correct one (as explained in bug 1003802 comment 6).
Attachment #8422229 - Flags: review?(benj) → feedback+
Here's an updated version including:
- Correction of the number test case -> Now we're only testing the RBitOr operation 
- Implementation of object test case
- Fixed MBitOr::canRecoverOnBailout() -> same implementation as MBitNot
- Style issues fixed, too

Managed to run jit-test-suite and global test suite on my computer, everything is green :)
Thanks!
Attachment #8422229 - Attachment is obsolete: true
Attachment #8422760 - Flags: review?(benj)
Btw, forgot to say, I rebased my local branch on origin/inbound.
Comment on attachment 8422760 [details] [diff] [review]
patch1008707v3.patch

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

Looks good! I rebased it for you, as it appears it actually wasn't rebased on inbound (the BitNot tests were not in the changeset context). Next time, please make sure to pull, update, rebase and refresh it ;).  I've also changed the object test, see below.

It seems this is your first contribution, thanks a lot for that! I pushed it to our try server, which will run the JIT tests on all supported platforms. If test return green, I'll push the changeset on inbound on your behalf and it will then get integrated in Firefox 32!

https://tbpl.mozilla.org/?tree=Try&rev=706891ee5eb9

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +56,5 @@
> +
> +var uceFault_bitor_object = eval(uneval(uceFault).replace('uceFault', 'uceFault_bitor_object'));
> +function rbitor_object(i) {
> +    var o = { valueOf: function() { return -100; } };
> +    var x = i | o;

It's tricky here, but this test won't raise the issue mentioned in the previous review (it's important the lambda implementing valueOf captures a value that changes afterwards). It should be this:

var t = i;
var o = { valueOf: function() { return t; } };
var x = o | -100;
// and so on.
Attachment #8422760 - Flags: review?(benj) → review+
Oops, sorry about the rebase. It seems I forgot to pull my repo.
Thanks for the support.
All green, let's get it landed! Thanks again for your contribution! Feel free to look at other bugs ;)

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b1625bff1ab
Assignee: nobody → jlevesy
Whiteboard: [good first bug][mentor=nbp][lang=c++] → [good first bug][mentor=bbouvier][lang=c++]
And by other bugs, if you want to take on other similar bugs, you can give a look at the ones bug 1003801 depends on. Otherwise, feel free to ask for more complex ones, here or on IRC ;)
I'll take a few more similar to this one. 
I need a deeper understanding of what's going on there before moving on difficult ones.
Btw, can you give me some pointers on documentation you consider helpful about this subject ? It would be great! Thanks :)
https://hg.mozilla.org/mozilla-central/rev/0b1625bff1ab
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.