Closed
Bug 1268518
Opened 8 years ago
Closed 8 years ago
Baldr: Implement rotations
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(2 files)
26.30 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
16.13 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
Pretty straightforward. Let's all welcome back MacroAssemblerARM::ma_ror/ma_rol from the dead (code)!
Attachment #8746561 -
Flags: review?(luke)
Assignee | ||
Comment 1•8 years ago
|
||
This passes the rotl/rotr tests from the spec repo, but let's have four more tests on our side. I've also changed the basic-integer.js file to make it less indented + test more things (constants) in general.
Attachment #8746565 -
Flags: review?(luke)
Assignee | ||
Comment 2•8 years ago
|
||
Comment on attachment 8746561 [details] [diff] [review] rotations.patch Review of attachment 8746561 [details] [diff] [review]: ----------------------------------------------------------------- Dan, Luke tells me you're back from pto. Welcome back!
Attachment #8746561 -
Flags: review?(luke) → review?(sunfish)
Assignee | ||
Updated•8 years ago
|
Attachment #8746565 -
Flags: review?(luke) → review?(sunfish)
Updated•8 years ago
|
Attachment #8746565 -
Flags: review?(sunfish) → review+
Comment 3•8 years ago
|
||
Comment on attachment 8746561 [details] [diff] [review] rotations.patch Review of attachment 8746561 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmIonCompile.cpp @@ +2533,5 @@ > + if (!EmitExpr(f, &input)) > + return false; > + > + MDefinition* count; > + if (!EmitExpr(f, &count)) May require rebase now :) ::: js/src/jit/arm/CodeGenerator-arm.cpp @@ +979,5 @@ > + Register creg = ToRegister(count); > + if (mir->isLeftRotate()) > + masm.ma_rol(creg, input, dest); > + else > + masm.ma_ror(creg, input, dest); The logic is almost identical here; is there a way to add rol/ror to the MacroAssembler and so the code-generator code could be shared? You'd of course need to use the target's word size to compute the 0x1F mask above but that seems doable.
Attachment #8746561 -
Flags: review?(sunfish) → review+
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #3) > ::: js/src/jit/arm/CodeGenerator-arm.cpp > @@ +979,5 @@ > > + Register creg = ToRegister(count); > > + if (mir->isLeftRotate()) > > + masm.ma_rol(creg, input, dest); > > + else > > + masm.ma_ror(creg, input, dest); > > The logic is almost identical here; is there a way to add rol/ror to the > MacroAssembler and so the code-generator code could be shared? You'd of > course need to use the target's word size to compute the 0x1F mask above but > that seems doable. Yes, can do. It doesn't seem easy to share this with LRotate64 though, hence just sharing the code paths for x86-shared and arm, which already is a nice improvement. Thanks for the review!
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae19bf92ae8e https://hg.mozilla.org/integration/mozilla-inbound/rev/217fc8589ae3
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae19bf92ae8e https://hg.mozilla.org/mozilla-central/rev/217fc8589ae3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•