These newly added tests shows that we have a latent bug somewhere in the 32-bit ARM JITs.
<rdar://problem/23636019>
The tests have been temporarily skipped in r192708: <http://trac.webkit.org/r192708>.
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.
Created attachment 271354 [details] proposed patch.
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.
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.
(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.
Thanks for the review. Landed in r196591: <http://trac.webkit.org/r196591>.