Closed
Bug 1009967
Opened 11 years ago
Closed 11 years ago
IonMonkey: Implement BitXor Recover Instruction
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: nbp, Assigned: inanc.seylan)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][mentor=nbp][lang=c++])
Attachments
(1 file, 1 obsolete file)
|
(deleted),
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
Implement RBitXor in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
|
Assignee | |
Comment 1•11 years ago
|
||
I would like to work on this bug; if it's ok.
|
Reporter | |
Updated•11 years ago
|
Assignee: nobody → acimilan
|
|
Assignee | |
Comment 2•11 years ago
|
||
An implementation of the recover code for BitXor and a test case.
Attachment #8422425 -
Flags: review?(nicolas.b.pierron)
|
|
Reporter | |
Comment 3•11 years ago
|
||
Comment on attachment 8422425 [details] [diff] [review]
rbitxor.patch
Review of attachment 8422425 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good, but …
::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +78,5 @@
>
> for (i = 0; i < 100; i++) {
> rbitnot_number(i);
> rbitnot_object(i);
> + rbitxor_number(i);
Add an object test case,
::: js/src/jit/MIR.h
@@ +3517,5 @@
> }
> void computeRange(TempAllocator &alloc);
> +
> + bool writeRecoverData(CompactBufferWriter &writer) const;
> + bool canRecoverOnBailout() const { return true; }
And have a look at MBinaryBitwiseInstruction ;)
Attachment #8422425 -
Flags: review?(nicolas.b.pierron) → feedback+
|
|
Assignee | |
Comment 4•11 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Comment on attachment 8422425 [details] [diff] [review]
> rbitxor.patch
>
> Add an object test case,
Done.
>
> ::: js/src/jit/MIR.h
>
> And have a look at MBinaryBitwiseInstruction ;)
Good point ;) Since I did not see any getAliasSet() method in MBitXor, my initial implementation returned true from canRecoverOnBailout().
|
|
Assignee | |
Updated•11 years ago
|
Attachment #8422425 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 5•11 years ago
|
||
Improvements to the previous version:
1. Test case for rbitxor_object.
2. A new canRecoverOnBailout() implementation for MBitXor so that it does not try to recover for objects.
Attachment #8422468 -
Flags: review?(nicolas.b.pierron)
|
|
Reporter | |
Comment 6•11 years ago
|
||
Comment on attachment 8422468 [details] [diff] [review]
rbitxor.patch
Review of attachment 8422468 [details] [diff] [review]:
-----------------------------------------------------------------
Nice work :)
Attachment #8422468 -
Flags: review?(nicolas.b.pierron) → review+
|
|
Assignee | |
Updated•11 years ago
|
Attachment #8422468 -
Flags: checkin+
|
|
Reporter | |
Comment 7•11 years ago
|
||
If you want your patch to be landed, do not set checkin+, but checkin?. Otherwise we might think that the patch is already landed. Also once the patch has a r+ from all reviewers without any fix-up needed, then you can add the checkin-needed keyword.
|
|
Assignee | |
Updated•11 years ago
|
Attachment #8422468 -
Flags: checkin+
|
|
Assignee | |
Updated•11 years ago
|
Keywords: checkin-needed
|
|
Assignee | |
Comment 8•11 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> If you want your patch to be landed, do not set checkin+, but checkin?.
> Otherwise we might think that the patch is already landed. Also once the
> patch has a r+ from all reviewers without any fix-up needed, then you can
> add the checkin-needed keyword.
Ok, thanks!
|
|
Reporter | |
Comment 9•11 years ago
|
||
|
||
Comment 11•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•