836373 - BaselineCompiler: IC for string equality

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom Schuster [:evilpie]]

Attachment: WIP

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

[Hannes Verschore [:h4writer]]

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

[Hannes Verschore [:h4writer]]

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 ...

Comment 3

[Tom Schuster [:evilpie]]

(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.

Comment 4

[Tom Schuster [:evilpie]]

Attachment: Changes for central

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.

Comment 5

[Tom Schuster [:evilpie]]

Attachment: baseline patch

Comment 6

[Tom Schuster [:evilpie]]

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

[Jan de Mooij [:jandem]]

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.

Comment 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[Tom Schuster [:evilpie]]

Attachment: baseline patch v2

Comment 10

[Jan de Mooij [:jandem]]

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

Comment 11

[Tom Schuster [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5223cda35aa

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f5223cda35aa

Comment 13

[Tom Schuster [:evilpie]]

I just realized that I can't actually use |right| as the output register.

Comment 14

[Tom Schuster [:evilpie]]

Attachment: baseline patch v3

I think this should be okay. Could you test this on x86, cross-building is not working for me right now.

Comment 15

[Tom Schuster [:evilpie]]

Attachment: v3

This should be the actual patch.

Comment 16

[Tom Schuster [:evilpie]]

Attachment: v4

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.

Comment 17

[Tom Schuster [:evilpie]]

Attachment: v4 actual patch

Comment 18

[Jan de Mooij [:jandem]]

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.

Comment 19

[Tom Schuster [:evilpie]]

https://hg.mozilla.org/projects/ionmonkey/rev/3dcd4bb17934