Closed
Bug 701957
Opened 13 years ago
Closed 13 years ago
IonMonkey: Compile JSOP_EQ & JSOP_NE
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: nbp, Assigned: nbp)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
18.16 KB,
patch
|
jandem
:
review+
mjrosenb
:
review+
|
Details | Diff | Splinter Review |
Necessary for benchmarks.
Assignee | ||
Updated•13 years ago
|
Assignee: general → nicolas.b.pierron
OS: Linux → All
Hardware: x86_64 → All
Summary: IonMonkey: Compile JSOP_EQ → IonMonkey: Compile JSOP_EQ & JSOP_NE
Assignee | ||
Comment 1•13 years ago
|
||
This patch is not yet complete on ARM because it does not handle NaNs. Value & String and any other types would likely be implemented with Bug 679804.
Attachment #575333 -
Flags: review?(sstangl)
Attachment #575333 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•13 years ago
|
||
Add VFP Condition flags. (Support for double with NaN checks ?)
Attachment #575333 -
Attachment is obsolete: true
Attachment #575333 -
Flags: review?(sstangl)
Attachment #575333 -
Flags: review?(jdemooij)
Attachment #575356 -
Flags: review?(mrosenberg)
Attachment #575356 -
Flags: review?(jdemooij)
Comment 3•13 years ago
|
||
Comment on attachment 575356 [details] [diff] [review] Implement JSOP_{STRICT,}{EQ,NE} for Double & Int Review of attachment 575356 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/arm/Assembler-arm.h @@ +859,5 @@ > + VFP_GreaterThan = GT, > + VFP_LessThanOrEqualOrUnordered = LE, > + VFP_LessThanOrEqual = LS, > + VFP_LessThanOrUnordered = LT, > + VFP_LessThan = CC, // MI is valid too. There is a trailing comma, some compilers complain about this. ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ -931,4 @@ > case JSOP_LE: > - as_cmp(ScratchRegister, O2Reg(ScratchRegister), Overflow); > - return Assembler::LessThanOrEqual; > - // GT and GE are naturally followed by "and not unordered..." This seems a bit sketchy, but after talking with Nicolas about it, he's convinced me that it is correct, and it seems to pass all of the tests.
Attachment #575356 -
Flags: review?(mrosenberg) → review+
Comment 4•13 years ago
|
||
Comment on attachment 575356 [details] [diff] [review] Implement JSOP_{STRICT,}{EQ,NE} for Double & Int Review of attachment 575356 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, most comments are style nits. ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp @@ +176,5 @@ > return true; > } > > void > +CodeGeneratorX86Shared::emitBranch(Assembler::Condition cond, MBasicBlock *mirTrue, MBasicBlock *mirFalse, NaNCond ifNaN) Nit: in SpiderMonkey code the character limit is 99 characters per line (80 in the rest of Mozilla) @@ +276,5 @@ > + Label noNaN; > + masm.j(Assembler::NoParity, &noNaN); > + masm.movl(Imm32(1), dest); > + masm.bind(&noNaN); > + } Can you combine the NaN_isTrue/NaN_isFalse cases, like this: -- setCC... if (ifNaN != NaN_unexpected) { Label noNaN; masm.j(Assembler::NoParity, &noNaN); if (ifNaN == NaN_isTrue) masm.movl(Imm32(1), dest); else masm.xorl(dest, dest); masm.bind(&noNaN); } -- Then you can also get rid of the "Label end" (or move it inside the else block). @@ +317,5 @@ > FloatRegister lhs = ToFloatRegister(comp->left()); > FloatRegister rhs = ToFloatRegister(comp->right()); > > Assembler::Condition cond = masm.compareDoubles(comp->jsop(), lhs, rhs); > + NaNCond ifNaN = cond == Assembler::NotEqual ? NaN_isTrue : NaN_isFalse; Nit: SM style is to put parentheses around the condition, like this: NaNCond ifNaN = (cond == Assembler::NotEqual) ? NaN_isTrue : NaN_isFalse; @@ +329,5 @@ > FloatRegister lhs = ToFloatRegister(comp->left()); > FloatRegister rhs = ToFloatRegister(comp->right()); > > Assembler::Condition cond = masm.compareDoubles(comp->jsop(), lhs, rhs); > + NaNCond ifNaN = cond == Assembler::NotEqual ? NaN_isTrue : NaN_isFalse; Same here. ::: js/src/jit-test/tests/ion/compareAll.js @@ +276,5 @@ > + compareSAndBranch("") > +]; > + > +print(result); > +assertEq(arraysEqual(result, expected), true); Nice tests.
Attachment #575356 -
Flags: review?(jdemooij) → review+
Comment 5•13 years ago
|
||
I just realized ComparePolicy calls CoercesToDouble, which returns true for MIRType_Undefined. If I understand this correctly, an expression like "undefined === undefined" (= true) will be lowered to "NaN === NaN" (= false). For strict equality we should only coerce int32 to double and never convert the other types (to something other than MIRType_Value). Another tricky case is: null == 0 -> false null <= 0 -> true
re: comment #5 - nice catch. yeah, here we should specialize but not coerce. looks good otherwise, drive-by style nit: NaN_IfTrue, etc (capitalize first letters)
Assignee | ||
Comment 7•13 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/789acacd0212 (In reply to Jan de Mooij (:jandem) from comment #5) > I just realized ComparePolicy calls CoercesToDouble, which returns true for > MIRType_Undefined. If I understand this correctly, an expression like > "undefined === undefined" (= true) will be lowered to "NaN === NaN" (= > false). For strict equality we should only coerce int32 to double and never > convert the other types (to something other than MIRType_Value). > > Another tricky case is: > > null == 0 -> false > null <= 0 -> true I'll forward this comment to Bug 679804.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Wait - is the case in comment #5 broken as-is with this patch? If so, this deserves a separate bug, with a quick plan to fix. We have to be careful about landing known correctness issues.
You need to log in
before you can comment on or make changes to this bug.
Description
•