Closed Bug 836373 Opened 9 years ago Closed 9 years ago

BaselineCompiler: IC for string equality

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Whiteboard: [leave open])

Attachments

(2 files, 6 obsolete files)

Attached patch WIP (obsolete) — 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 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()));
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: general → evilpies
Summary: BaselineCompiler: Stub for string equality → BaselineCompiler: IC for string equality
(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.
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)
Attached patch baseline patch (obsolete) — Splinter Review
Attachment #710902 - Flags: review?(jdemooij)
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 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 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)
Attached patch baseline patch v2 (obsolete) — Splinter Review
Attachment #710902 - Attachment is obsolete: true
Attachment #711367 - Flags: review?(jdemooij)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5223cda35aa
Status: NEW → ASSIGNED
Whiteboard: [leave open]
I just realized that I can't actually use |right| as the output register.
Attached patch baseline patch v3 (obsolete) — Splinter Review
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
Attachment #711941 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
This should be the actual patch.
Attachment #711956 - Flags: review?(jdemooij)
Attached patch v4 (obsolete) — Splinter Review
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)
Attached patch v4 actual patchSplinter Review
Attachment #712478 - Attachment is obsolete: true
Attachment #712478 - Flags: review?(jdemooij)
Attachment #712502 - Flags: review?(jdemooij)
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+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.