Closed
Bug 681185
Opened 13 years ago
Closed 13 years ago
IonMonkey: implement shifts
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: h4writer, Assigned: h4writer)
Details
Attachments
(1 file)
32.09 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
implement JSOP_LSH,JSOP_RSH and JSOP_URSH
Assignee | ||
Comment 1•13 years ago
|
||
implementation + tests
Assignee: general → hv1989
Attachment #555039 -
Flags: review?(dvander)
Comment on attachment 555039 [details] [diff] [review] implement JSOP_LSH, JSOP_RSH and JSOP_URSH Review of attachment 555039 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/MIR.h @@ +1148,5 @@ > + return this; > + } > + > + bool canBeNegative() { > + // solution is only negative when lhs < 0 and rhs & 0x1f == 0 Logic below looks good, but the name threw me off. How about "canOverflow"? ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp @@ +362,5 @@ > + // But we need to bring the output back again from UINT32 to INT32. > + // Both representation overlap each other in the positive numbers. (in INT32) > + // So there is only a problem when solution (in INT32) is negative. > + if (ursh->canBeNegative()) { > + masm.cmpl(Imm32(0), ToRegister(lhs)); Nice, glad you caught this case and thanks for explaining it. Are the inputs to cmpl() swapped? It reads as bailoutIf(0 < reg) ::: js/src/ion/x86/Assembler-x86.h @@ +288,5 @@ > + } > + void sarl(const Register shift, const Register &dest) { > + JS_ASSERT(shift == ecx); > + masm.sarl_CLr(dest.code()); > + } How about instead of sarl(reg, reg) we call it sarl_cl(reg)? That way it's clear that it uses ecx but doesn't take in a bogo-parameter. Same with shrl/shll. Then you can share these functions in Assembler-x86-shared. ::: js/src/ion/x86/Lowering-x86.h @@ +75,5 @@ > bool assignSnapshot(LInstruction *ins); > void lowerUntypedPhiInput(MPhi *phi, uint32 inputPosition, LBlock *block, size_t lirIndex); > bool defineUntypedPhi(MPhi *phi, size_t lirIndex); > > + bool lowerForShifting(LInstructionHelper<1, 2, 0> *ins, MDefinition *mir, MDefinition *lhs, s/Shifting/Shift
Attachment #555039 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 3•13 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/c1ec835d0f9d
Assignee | ||
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•