Closed
Bug 679493
Opened 13 years ago
Closed 13 years ago
IonMonkey: implement JSOP_MUL
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: h4writer, Assigned: h4writer)
Details
Attachments
(5 files, 1 obsolete file)
12.58 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
4.30 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
4.47 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
3.66 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
implement the opcode JSOP_NEG in ionmonkey
Assignee | ||
Comment 1•13 years ago
|
||
Implements base structure + LNegI. Next (and last part) is LNegD.
Few issues encountered when making this patch:
- LSRA fails when foldsto returns a MConstant:Double with an error. Greedy works so I think this is an issue with LSRA. Should I file a bug for an issue that only "not-checked-in" code can trigger?
>> Assertion failure: a.isFloatReg(), at ../ion/shared/CodeGenerator-shared-inl.h:70
- Noticed JSOP_ADD has faults in his constant folding code. Will file bug for this.
Assignee: general → hv1989
Attachment #553581 -
Flags: review?(dvander)
Assignee | ||
Comment 2•13 years ago
|
||
Comment on attachment 553581 [details] [diff] [review] part 1: implement base + LNegI Review of attachment 553581 [details] [diff] [review]: ----------------------------------------------------------------- After talk in irc with dvander it's better to have a different approach: Adjusting JSOP_NEG into MUL(x,-1).
Attachment #553581 -
Flags: review?(dvander) → review-
Assignee | ||
Comment 3•13 years ago
|
||
note: - issue 1: is normal. We can't change the type anymore during that phase. - issue 2: solved by bug #678625
Assignee | ||
Updated•13 years ago
|
Summary: IonMonkey: implement JSOP_NEG → IonMonkey: implement JSOP_MUL
Assignee | ||
Comment 4•13 years ago
|
||
Using this bug for JSOP_MUL now. TODO: 1) redirect JSOP_NEG to JSOP_MUL. 2) add MMul to LMulI to know if zero/overflow tests are needed. 3) make work with doubles
Attachment #553581 -
Attachment is obsolete: true
Assignee | ||
Comment 5•13 years ago
|
||
implements 1) and 3).
Assignee | ||
Comment 6•13 years ago
|
||
This adds seperate overflow and zero checks. The checks are on by default and there are no function yet to disable a check. But it gives a foundation for later. When the idea of the fallible patch (dvander: see mail) get's approved, then this will change/simplify a little bit.
Assignee | ||
Updated•13 years ago
|
Attachment #554233 -
Flags: review?(dvander)
Assignee | ||
Updated•13 years ago
|
Attachment #554262 -
Flags: review?(dvander)
Assignee | ||
Updated•13 years ago
|
Attachment #554269 -
Flags: review?(dvander)
Comment on attachment 554233 [details] [diff] [review] Part 1 - basics, JSOP_MUL, MMul, LMulI and tests Review of attachment 554233 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just some nits: ::: js/src/ion/shared/Assembler-x86-shared.h @@ +482,5 @@ > + } > + void mull(const Register &src, const Register &dest) { > + masm.imull_rr(src.code(), dest.code()); > + } > + void mull(const Operand &src, const Register &dest) { Could you rename these to imull? "mul" on x86 is an unsigned multiply. ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp @@ +219,5 @@ > + if (rhs->isConstant()) { > + // Bailout on -0.0 > + int32 constant = ToInt32(rhs); > + if (ins->snapshot() && constant <= 0) { > + Assembler::Condition bailoutCond = (constant == 0)?Assembler::LessThan : Assembler::Zero; Give the ? some breathing room: ...) ? Assembler:: ... Also, though they're the same thing, Assembler::Equal is a little clearer when used with cmpl(). Nice work on simplifying this check, btw :) @@ +225,5 @@ > + if (bailoutIf(bailoutCond, ins->snapshot())) > + return false; > + } > + > + masm.mull(Imm32(ToInt32(rhs)), ToRegister(lhs)); You could peephole optimize here, if you wanted - neg for -1, xor for 0, add for 2. @@ +238,5 @@ > + if (ins->snapshot() && !bailoutIf(Assembler::Overflow, ins->snapshot())) > + return false; > + > + // Bailout on 0 (could be -0.0) > + if (ins->snapshot()){ Space between ) and { @@ +241,5 @@ > + // Bailout on 0 (could be -0.0) > + if (ins->snapshot()){ > + masm.cmpl(Imm32(0), ToRegister(lhs)); > + if (!bailoutIf(Assembler::Zero, ins->snapshot())) > + return false; This is what our past two JITs do - aggressively deoptimize on a zero result. Seems okay, I don't think it caused problems before. Fixing it would mean extra guards or more register pressure which would be sad.
Attachment #554233 -
Flags: review?(dvander) → review+
Updated•13 years ago
|
Attachment #554262 -
Flags: review?(dvander) → review+
Comment on attachment 554269 [details] [diff] [review] Part 3: add seperate Overflow/Zero checks Review of attachment 554269 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp @@ +219,5 @@ > > if (rhs->isConstant()) { > // Bailout on -0.0 > int32 constant = ToInt32(rhs); > + if (mul->needsZero() && constant <= 0) { s/needsZero/canBeNegativeZero/ @@ +229,5 @@ > > masm.mull(Imm32(ToInt32(rhs)), ToRegister(lhs)); > > // Bailout on overflow > + if (mul->needsOverflow() && !bailoutIf(Assembler::Overflow, ins->snapshot())) s/needsOverflow/canOverflow
Attachment #554269 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 9•13 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/b7c3e89e50cc http://hg.mozilla.org/projects/ionmonkey/rev/0b8dece77de1 http://hg.mozilla.org/projects/ionmonkey/rev/71de4f9ef039 (In reply to David Anderson [:dvander] from comment #7) > You could peephole optimize here, if you wanted - neg for -1, xor for 0, add > for 2. Sure, I'll do that. Do we want shifts for constants that are multiples of 2? Testcases give the following error that will get fixes by patch in bug #678798: Assertion failure: ins->valueNumber() != 0 at js/src/ion/ValueNumbering.cpp:273
I think shift doesn't set the overflow flag
Assignee | ||
Comment 11•13 years ago
|
||
The requested peephole optimizations. shl indeed only sets overflow bit with 1-bit shift :(. And cycle count of cmp + shl is almost the same as just running muli. So no shift optimizations.
Attachment #554296 -
Flags: review?(dvander)
Comment on attachment 554296 [details] [diff] [review] part 4: peephole optimizations Review of attachment 554296 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp @@ +241,5 @@ > return false; > } > > + // Should have been folded out. > + JS_ASSERT(constant != 1); I wouldn't rely on this - we might want to turn off GVN for the baseline compiler.
Attachment #554296 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 13•13 years ago
|
||
This patch goes on top of part4! (In reply to David Anderson [:dvander] from comment #12) > ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp > @@ +241,5 @@ > > return false; > > } > > > > + // Should have been folded out. > > + JS_ASSERT(constant != 1); > > I wouldn't rely on this - we might want to turn off GVN for the baseline > compiler. 1) I see. In that case I added the optimization to NOP. 2) I managed to add the shll optimization ;) (with performance increase, I suspect 3x). Only if constant is positive and canOverflow is false, though.
Attachment #554473 -
Flags: review?(dvander)
Updated•13 years ago
|
Attachment #554473 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 14•13 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/bd11f4fae27a
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
•