RESOLVED FIXED 151514
[ARMv7] stress/op_rshift.js and stress/op_urshift.js are failing. 
https://bugs.webkit.org/show_bug.cgi?id=151514
Summary [ARMv7] stress/op_rshift.js and stress/op_urshift.js are failing.
Mark Lam
Reported 2015-11-20 14:13:18 PST
These newly added tests shows that we have a latent bug somewhere in the 32-bit ARM JITs.
Attachments
proposed patch. (4.91 KB, patch)
2016-02-15 12:10 PST, Mark Lam 		fpizlo: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2015-11-20 14:14:36 PST
<rdar://problem/23636019>
Mark Lam
Comment 2 2015-11-20 15:47:03 PST
The tests have been temporarily skipped in r192708: <http://trac.webkit.org/r192708>.
Mark Lam
Comment 3 2016-02-15 11:57:18 PST
The issue turns out to be trivial: on ARMv7 (and traditional ARM too), arithmetic shift right (ASR) and logical shift right (LSR) takes an immediate shift amount from 1-32. See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0204j/Cjacbgca.html. An immediate shift amount of 0 is interpreted as a shift of 32 bits. Meanwhile, our assembler is expecting the immediate shift value to be between 0-31. As a result, a shift amount of 0 is being wrongly encoded with 0 bits which means shift right by 32 bits. The fix is to check if the shift amount is 0, and if so, emit a move. Else, emit the right shift as usual. This issue does not affect left shifts, as the immediate shift amount for left shifts is between 0-31 as our JIT expects.
Mark Lam
Comment 4 2016-02-15 12:10:42 PST
Created attachment 271354 [details] proposed patch.
Filip Pizlo
Comment 5 2016-02-15 12:11:45 PST
Comment on attachment 271354 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=271354&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:278 > + move(src, dest); This is wrong. Please use a zero-extending move. > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:304 > + move(src, dest); Ditto. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:404 > + move(src, dest); Ditto. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:432 > + move(src, dest); Ditto.
Mark Lam
Comment 6 2016-02-15 12:14:16 PST
These changes are in MacroAssemblerARM.h and MacroAssemblerARMv7.h. These operations are 32-bit only. Why do we need a zero extending move? (In reply to comment #5) > Comment on attachment 271354 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271354&action=review > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:278 > > + move(src, dest); > > This is wrong. Please use a zero-extending move. > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:304 > > + move(src, dest); > > Ditto. > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:404 > > + move(src, dest); > > Ditto. > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:432 > > + move(src, dest); > > Ditto.
Filip Pizlo
Comment 7 2016-02-15 12:21:22 PST
(In reply to comment #6) > These changes are in MacroAssemblerARM.h and MacroAssemblerARMv7.h. These > operations are 32-bit only. Why do we need a zero extending move? > > (In reply to comment #5) > > Comment on attachment 271354 [details] > > proposed patch. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=271354&action=review > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:278 > > > + move(src, dest); > > > > This is wrong. Please use a zero-extending move. > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:304 > > > + move(src, dest); > > > > Ditto. > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:404 > > > + move(src, dest); > > > > Ditto. > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:432 > > > + move(src, dest); > > > > Ditto. OMG you're right. Please ignore me.
Mark Lam
Comment 8 2016-02-15 13:09:46 PST
Thanks for the review. Landed in r196591: <http://trac.webkit.org/r196591>.
Note You need to log in before you can comment on or make changes to this bug.