Closed
Bug 836373
Opened 10 years ago
Closed 10 years ago
BaselineCompiler: IC for string equality
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: evilpie, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
(Whiteboard: [leave open])
Attachments
(2 files, 6 obsolete files)
22.28 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
4.66 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
This reusing code from MCompareString and putting it into the MacroAssembler. Still a WIP, because I need to split off the IonMonkey changes.
Comment 1•10 years ago
|
||
Comment on attachment 708198 [details] [diff] [review] WIP Review of attachment 708198 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/x86/CodeGenerator-x86.cpp @@ +385,2 @@ > JS_ASSERT(mir->jsop() == JSOP_EQ || mir->jsop() == JSOP_STRICTEQ || > mir->jsop() == JSOP_NE || mir->jsop() == JSOP_STRICTNE); JS_ASSERT(IsEqualityOp(mir->jsop()));
Comment 2•10 years ago
|
||
FYI bug 828119 landed. Now the changes to visitCompareS you did with using "masm.compareStrings", you'll have to do them in emitCompareS now ... Just a note, but "masm.compareStrings" actually looks like a wrong name? Because the strings don't get compared fully... Don't want people to use it, because they think it does the full string compare ...
Assignee | ||
Updated•10 years ago
|
Assignee: general → evilpies
Summary: BaselineCompiler: Stub for string equality → BaselineCompiler: IC for string equality
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #2) > FYI bug 828119 landed. Now the changes to visitCompareS you did with using > "masm.compareStrings", you'll have to do them in emitCompareS now ... > > Just a note, but "masm.compareStrings" actually looks like a wrong name? > Because the strings don't get compared fully... Don't want people to use it, > because they think it does the full string compare ... I think it's okay. |newGCThing| is kind of similar. Plus there is the explicit fail parameter.
Assignee | ||
Comment 4•10 years ago
|
||
This moves code around so that we can actually use it in baseline. It is kind of painful, because we have so much in CodeGenerator and not MacroAssembler.
Attachment #708198 -
Attachment is obsolete: true
Attachment #710882 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #710902 -
Flags: review?(jdemooij)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 710902 [details] [diff] [review] baseline patch Review of attachment 710902 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/BaselineIC.cpp @@ +1218,5 @@ > stub->addNewStub(doubleStub); > return true; > } > > + if (lhs.isString() && rhs.isString() && IsEqualityOp(op) && !stub->hasStub(ICStub::GetElem_String)) { Oh, yeah, going to fix this.
Comment 7•10 years ago
|
||
Comment on attachment 710882 [details] [diff] [review] Changes for central Review of attachment 710882 [details] [diff] [review]: ----------------------------------------------------------------- Very nice that we can reuse the inline string comparison path. ::: js/src/ion/arm/MacroAssembler-arm.h @@ +1100,5 @@ > + void > + emitSet(Assembler::Condition cond, const Register &dest) > + { > + masm.ma_mov(Imm32(0), dest); > + masm.ma_mov(Imm32(1), dest, NoSetCond, cond); Remove the "masm." here.
Attachment #710882 -
Flags: review?(jdemooij) → review+
Comment 8•10 years ago
|
||
Comment on attachment 710902 [details] [diff] [review] baseline patch Review of attachment 710902 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/BaselineIC.cpp @@ +1266,5 @@ > + Register right = masm.extractString(R1, ExtractTemp1); > + > + GeneralRegisterSet regs(availableGeneralRegs(2)); > + Register scratchReg = regs.takeAny(); > + Register scratchReg2 = regs.takeAny(); This will fail on x86, we have only one register available after we take R0 and R1. The 6th register holds the stub pointer and we can't clobber that either. The best way to solve it I think is to take only the "left" and "right" registers from "regs" and allow clobbering the type tags (and box again after the failure lable). ::: js/src/ion/BaselineIC.h @@ +1307,5 @@ > +{ > + friend class ICStubSpace; > + > + ICCompare_String(IonCode *stubCode) > + : ICStub(ICStub::Compare_Double, stubCode) Compare_String here. @@ +1321,5 @@ > + bool generateStubCode(MacroAssembler &masm); > + > + public: > + Compiler(JSContext *cx, JSOp op) > + : ICMultiStubCompiler(cx, ICStub::Compare_Double, op) And here.
Attachment #710902 -
Flags: review?(jdemooij)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #710902 -
Attachment is obsolete: true
Attachment #711367 -
Flags: review?(jdemooij)
Comment 10•10 years ago
|
||
Comment on attachment 711367 [details] [diff] [review] baseline patch v2 Review of attachment 711367 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding this, r=me with comments addressed. ::: js/src/ion/BaselineCompiler.cpp @@ +1,1 @@ > +f/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- Nit: remove the "f" at the start of this line ::: js/src/ion/BaselineIC.cpp @@ +1271,5 @@ > + Register scratchReg = regs.takeAny(); > + > + // x86 doesn't have the luxury of a second scratch. > + if (right != ExtractTemp1) > + masm.push(right); ARM also has more registers and it would be nice if we could avoid the push/pop there, so can you instead do something like this: if (regs.empty()) { scratchReg2 = right; masm.push(right); } else { scratchReg2 = regs.takeAny(); } And further down you can masm.pop(..) if scratchReg2 == right
Attachment #711367 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5223cda35aa
Status: NEW → ASSIGNED
Whiteboard: [leave open]
Assignee | ||
Comment 13•10 years ago
|
||
I just realized that I can't actually use |right| as the output register.
Assignee | ||
Comment 14•10 years ago
|
||
I think this should be okay. Could you test this on x86, cross-building is not working for me right now.
Attachment #711367 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #711941 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
This should be the actual patch.
Attachment #711956 -
Flags: review?(jdemooij)
Assignee | ||
Comment 16•10 years ago
|
||
Thanks to Jan's local testing on x86, we found a problem in the previous patch. We poped the registered even in the case were we hadn't pushed yet.
Attachment #711956 -
Attachment is obsolete: true
Attachment #711956 -
Flags: review?(jdemooij)
Attachment #712478 -
Flags: review?(jdemooij)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #712478 -
Attachment is obsolete: true
Attachment #712478 -
Flags: review?(jdemooij)
Attachment #712502 -
Flags: review?(jdemooij)
Comment 18•10 years ago
|
||
Comment on attachment 712502 [details] [diff] [review] v4 actual patch Review of attachment 712502 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/ion/BaselineIC.cpp @@ +1301,5 @@ > + masm.pop(BaselineStubReg); > + EmitReturnFromIC(masm); > + > + > + masm.bind(&complexCompare); Nit: inlineCompareFailed, and remove one of the newlines while you are here.
Attachment #712502 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 19•10 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/3dcd4bb17934
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•