Closed Bug 1341292 Opened 3 years ago Closed 2 years ago

MIPS32/MIPS64: Fix assert related to incorrect use of scratch registers

Categories

(Core :: JavaScript Engine: JIT, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ivica.bogosavljevic, Assigned: ivica.bogosavljevic)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch incorrect_use_scratch.patch (obsolete) — Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

There are two unused asserts in MoveEmitterMIPS[32|64]::emitDoubleMove that cause failures in some tests. The asserts check if the source or destination register is scratch register, however scratch register is actually used only for memory to memory move.

Test that are fixed by this patch (jit-tests):
asm.js/testFloat32.js
SIMD/uconvert.js
Flags: needinfo?(lhansen)
Comment on attachment 8839469 [details] [diff] [review]
incorrect_use_scratch.patch

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

Sure, looks good.  We can probably just land this, right?

If you think you want some feedback on a patch but don't want to ask for review yet, then you can use the "feedback?" flag on the patch in bugzilla.
Attachment #8839469 - Flags: review+
Flags: needinfo?(lhansen)
Assignee: nobody → ivica.bogosavljevic
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Lars T Hansen [:lth] from comment #1)
> Comment on attachment 8839469 [details] [diff] [review]
> incorrect_use_scratch.patch
> 
> Review of attachment 8839469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sure, looks good.  We can probably just land this, right?
> 
> If you think you want some feedback on a patch but don't want to ask for
> review yet, then you can use the "feedback?" flag on the patch in bugzilla.

It is safe to land this.
Priority: -- → P5
This bug is still causing some tests to fail. I adjusted the patch, please take a look. For mips64 this was fixed in Bug 1389401 by using second scratch register but actually the asserts should be removed because if from and to operands are memory addresses then neither can be a scratch register.
Attachment #8904527 - Flags: review?(lhansen)
Attachment #8904527 - Flags: checkin+
Also the other patch is now obsolete, but I cannot change it's status.
Attachment #8839469 - Attachment is obsolete: true
Attachment #8904527 - Flags: review?(lhansen)
Attachment #8904527 - Flags: review+
Attachment #8904527 - Flags: checkin+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5412efabadb
MIPS: Remove redundant asserts. r=lth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c5412efabadb
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.