Bug 151514 - [ARMv7] stress/op_rshift.js and stress/op_urshift.js are failing.
Summary: [ARMv7] stress/op_rshift.js and stress/op_urshift.js are failing.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks: 108645
  Show dependency treegraph
 
Reported: 2015-11-20 14:13 PST by Mark Lam
Modified: 2016-02-15 13:09 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (4.91 KB, patch)
2016-02-15 12:10 PST, Mark Lam
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 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.
Comment 1 Radar WebKit Bug Importer 2015-11-20 14:14:36 PST
<rdar://problem/23636019>
Comment 2 Mark Lam 2015-11-20 15:47:03 PST
The tests have been temporarily skipped in r192708: <http://trac.webkit.org/r192708>.
Comment 3 Mark Lam 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.
Comment 4 Mark Lam 2016-02-15 12:10:42 PST
Created attachment 271354 [details]
proposed patch.
Comment 5 Filip Pizlo 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.
Comment 6 Mark Lam 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.
Comment 7 Filip Pizlo 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.
Comment 8 Mark Lam 2016-02-15 13:09:46 PST
Thanks for the review.  Landed in r196591: <http://trac.webkit.org/r196591>.