Closed Bug 1011540 Opened 6 years ago Closed 6 years ago

IonMonkey: Implement Mul Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: nbp, Assigned: sankha)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 3 obsolete files)

Implement RMul in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
Attached patch patchv1 (obsolete) — Splinter Review
Attachment #8424464 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8424464 [details] [diff] [review]
patchv1

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

(Canceled review, fix RSub and this patch too before fixing other bugs)
Attachment #8424464 - Flags: review?(nicolas.b.pierron)
Can I assign this bug to Sankha if he's working on it?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Mike Hoye [:mhoye] from comment #3)
> Can I assign this bug to Sankha if he's working on it?

Bug 1011539 is not finished yet, and I don't know if he is still working on these bugs.
Flags: needinfo?(nicolas.b.pierron)
Attached patch patchv2 (obsolete) — Splinter Review
Attachment #8424464 - Attachment is obsolete: true
Attachment #8435730 - Flags: review?(nicolas.b.pierron)
Attached image iongraph output
This shows the iongraph output for RMul instruction in the rmul-float function. The mul instructions are coloured in gray like resume points.
Attached patch patchv2 (obsolete) — Splinter Review
Attachment #8435730 - Attachment is obsolete: true
Attachment #8435730 - Flags: review?(nicolas.b.pierron)
Attachment #8435736 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8435736 [details] [diff] [review]
patchv2

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

Nice :)

Fix the two cases and check your patch on Try.
If you do not have commit access yet, upload a new patch with the recommended fixes, such as Sheriff/I can land the definitive versions.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +195,5 @@
> +var uceFault_mul_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_mul_number'));
> +function rmul_number(i) {
> +    var x = 1 * i;
> +    if (uceFault_mul_number(i) || uceFault_mul_number(i))
> +        assertEq(x, 99  /* = 1 * 99 */);

Take another value than 1, as it is possible that we just remove this multiplication (or should remove it).

::: js/src/jit/MIR.h
@@ +4406,5 @@
>      Mode mode() const { return mode_; }
> +
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +    bool canRecoverOnBailout() const {
> +        return specialization_ != MIRType_None;

Use a similar condition as the getAliasSet method of this MIR (in this case it is inherited)
Attachment #8435736 - Flags: review?(nicolas.b.pierron) → review+
Attached patch patch v3Splinter Review
Putting the r+ from previous patch.

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=2ab9708a5f4f
Assignee: nobody → sankha93
Attachment #8435736 - Attachment is obsolete: true
Attachment #8436100 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e074affd0a3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.