Closed Bug 1008110 Opened 10 years ago Closed 10 years ago

IonMonkey: Implement BitAnd Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: nbp, Assigned: inanc.seylan)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 2 obsolete files)

Implement RBitAnd in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanations.
Attached patch rbitnot.patch (obsolete) — Splinter Review
Hi, 

Here's an implementation of the RBitNot recover instruction. 

I've tested it in a separate .js file that is not in the repository. The test logic is as in dce-with-rinstructions.js. It recovers well, i.e., the assertion is valid and I see the gray text in the generated iongraph output.
Sorry, this patch is supposed to be for the bug

https://bugzilla.mozilla.org/show_bug.cgi?id=1003802
Comment on attachment 8420624 [details] [diff] [review]
rbitnot.patch

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

This patch looks good :)
Add a test case for it and we would integrate it in Firefox ;)

::: js/src/jit/Recover.cpp
@@ +208,5 @@
> +
> +RBitNot::RBitNot(CompactBufferReader &reader)
> +{
> +
> +}

style-nit: remove this empty line.

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

nit: I suggest we add it just before the Add and after the ResumePoint, to group them by categories, as done in Bug 1003801
Attachment #8420624 - Flags: feedback+
Comment on attachment 8420624 [details] [diff] [review]
rbitnot.patch

Check the bug https://bugzilla.mozilla.org/show_bug.cgi?id=1003802 instead.
Attachment #8420624 - Attachment is obsolete: true
Attached patch patchv1 (obsolete) — Splinter Review
Attachment #8424446 - Flags: review?(nicolas.b.pierron)
Attached patch patchv1Splinter Review
Attachment #8424446 - Attachment is obsolete: true
Attachment #8424446 - Flags: review?(nicolas.b.pierron)
Attachment #8424449 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8424449 [details] [diff] [review]
patchv1

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

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +114,5 @@
>      return i;
>  }
>  
> +var uceFault_and_object = eval(uneval(uceFault).replace('uceFault', 'uceFault_and_object'));
> +function rbitand_object(i) {

Group these functions, and follow the same order as listed in Bug 1003801 comment 0.

::: js/src/jit/Recover.h
@@ +22,1 @@
>      _(BitNot)                                   \

Follow the order, as listed in Bug 1003801 comment 0.
Attachment #8424449 - Flags: review?(nicolas.b.pierron)
Attached patch rbitand.patchSplinter Review
Attachment #8428608 - Flags: review?(nicolas.b.pierron)
Fixed the small problems in Sankha's patch and asserted that lhs and rhs are not objects in RBitAnd::recover()
Comment on attachment 8428608 [details] [diff] [review]
rbitand.patch

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

This patch sounds good, can you send it to try to check[1] JS failures, such as rooting issues, Valgrind and Asan builds, and run the jittests and the jsreftests.
If you need to request Try access, you can needinfo me and/or benj.

Once the all the entries are green, add the checkin-needed keyword in the bug, such as a sheriff or me can push the bug for you ;)

[1] http://trychooser.pub.build.mozilla.org/
Attachment #8428608 - Flags: review?(nicolas.b.pierron) → review+
Assignee: nobody → inanc.seylan
Attachment #8428608 - Flags: checkin?(nicolas.b.pierron)
Comment on attachment 8428608 [details] [diff] [review]
rbitand.patch

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

Sorry for the delay, I forgot to push this patch after doing the review.

https://tbpl.mozilla.org/?tree=Try&rev=18b49b73f250
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0c18b3fead
Attachment #8428608 - Flags: checkin?(nicolas.b.pierron) → checkin+
https://hg.mozilla.org/mozilla-central/rev/7b0c18b3fead
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.