1434783 - Remove extractString

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Steve Fink [:sfink] [:s:]]

Over in bug 903519, I was confused by the (partial) presence of extractString as opposed to unboxString or unboxNonDouble. It seems like it would be nice to just eliminate extractString entirely if possible.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Remove extractString

Ok, the remaining messiness that I'm aware of is in ICCompare_String::Compiler::generateStubCode. AFAIK, this patch works fine, but it exposes what seems (to me) to be a dodgy assumption:

    Register left = masm.extractString(R0, ExtractTemp0);
    Register right = masm.extractString(R1, ExtractTemp1);

This code on NUNBOX32 will set left=R0.payloadReg(), right=R1.payloadReg(), which seems fine. But on 64-bit, it will set left=ExtractTemp0, right=R1.ExtractTemp1, and then depend on masm.compareStrings not using either of those temporaries. Which seems rather dangerous. The rewrite in this bug makes the assumption more obvious, but also extends the assumption to 32-bit. So if masm.compareStrings uses either of those registers on 32-bit, then this will fail. And although I think the assumption is currently correct, I see nothing checking it.

The second nastiness in this patch is the use of valueOperand.scratchReg() to mean "a payload register that is part or all of this ValueOperand, so either NUNBOX32 vop.payloadReg() or PUNBOX64 vop.valueReg()". ValueOperand::scratchReg() does what I want, but it isn't documented, and I don't understand what "scratch" means in this context. If valueReg() were named payloadReg(), I would have used that. Is there a way to more clearly say "end the live range of this ValueOperand, and start the live range of this register that holds something I'm pulling something out of the ValueOperand when I kill its live range"? Or is that precisely what scratchReg() is meant to convey?

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8947308 [details] [diff] [review]
Remove extractString

Review of attachment 8947308 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CodeGenerator.cpp
@@ +7405,5 @@
>  
>      masm.bind(&string);
> +
> +    Register left = leftV.scratchReg();
> +    masm.unboxNonDouble(leftV, left, JSVAL_TYPE_STRING);

On 64-bit platforms this will change what's in |leftV| and that might confuse other instructions using this register.

::: js/src/jit/SharedIC.cpp
@@ +1636,5 @@
>      MOZ_ASSERT(IsEqualityOp(op));
>  
> +    // This code depends on masm.compareStrings not using ExtractTemp[01].
> +    Register left = ExtractTemp0;
> +    Register right = ExtractTemp1;

This won't work on 32-bit platforms since ExtractTemp0/1 are defined as InvalidReg there.

One thing we could do is unbox in R0.scratchReg() and R1.scratchReg(), then on the failure path after we unbox we could use masm.tagValue(R0, JSVAL_TYPE_STRING, left); to restore it.

Longer term we should convert this IC to CacheIR - it doesn't use extract* and handles this stuff automatically.

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Comment on attachment 8947308 [details] [diff] [review]
> Remove extractString
> 
> Review of attachment 8947308 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/CodeGenerator.cpp
> @@ +7405,5 @@
> >  
> >      masm.bind(&string);
> > +
> > +    Register left = leftV.scratchReg();
> > +    masm.unboxNonDouble(leftV, left, JSVAL_TYPE_STRING);
> 
> On 64-bit platforms this will change what's in |leftV| and that might
> confuse other instructions using this register.

Oh, you're right. I mixed this up with the other case where the ValueOperand was from regs.takeAnyValue(). Bleh, sorry.

> ::: js/src/jit/SharedIC.cpp
> @@ +1636,5 @@
> >      MOZ_ASSERT(IsEqualityOp(op));
> >  
> > +    // This code depends on masm.compareStrings not using ExtractTemp[01].
> > +    Register left = ExtractTemp0;
> > +    Register right = ExtractTemp1;
> 
> This won't work on 32-bit platforms since ExtractTemp0/1 are defined as
> InvalidReg there.

Ah, that explains things.

> One thing we could do is unbox in R0.scratchReg() and R1.scratchReg(), then
> on the failure path after we unbox we could use masm.tagValue(R0,
> JSVAL_TYPE_STRING, left); to restore it.

Heh, that's clever.

> Longer term we should convert this IC to CacheIR - it doesn't use extract*
> and handles this stuff automatically.

Yeah, I don't know anything that.

Is this patch worth finishing up? I launched into it for bogus reasons triggered by unexpected rebase breakage, and I only continued because I've always been confused about extract* vs unboxNonDouble and it seemed nice to get rid of it. But it seems like the uses will eventually go away anyway.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Remove extractString

Comment 5

[Steve Fink [:sfink] [:s:]]

I kind of wanted to better learn how all this stuff works, so I got it passing locally on 32-bit and 64-bit. May as well see what you think.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8947550 [details] [diff] [review]
Remove extractString

Review of attachment 8947550 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this! r=me with comments below addressed.

::: js/src/jit/CodeGenerator.cpp
@@ +7276,5 @@
>      masm.jump(&done);
>  
>      masm.bind(&string);
> +    masm.unboxNonDouble(leftV, output, JSVAL_TYPE_STRING);
> +    emitCompareS(lir, op, output, right, output);

Hm is this safe, considering emitCompareS calls masm.compareStrings where we do this (it will clobber the result/left register, then fail):

https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/js/src/jit/MacroAssembler.cpp#1362-1363

I think it would be better/safer to keep a temp register, on all platforms, and then just do:

  masm.unboxString(leftV, temp);

I also think masm.compareStrings should

  MOZ_ASSERT(left != result); 
  MOZ_ASSERT(right != result);

::: js/src/jit/arm64/MacroAssembler-arm64.h
@@ +1417,5 @@
>  
>      void unboxString(const ValueOperand& operand, Register dest) {
>          unboxNonDouble(operand, dest, JSVAL_TYPE_STRING);
>      }
> +    void unboxString(Register src, Register dest) {

Remove this one (we should use the unboxString overload that takes src as ValueOperand).

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Remove extractString

Ok, I only went back to this because it came up in my list of unlanded bugs, and I thought it'd be easy to get rid of.

Sadly, extractString() seems to have a purpose: it hides the weird temp register hackery that makes LStrictCompareS work on ARM as well as everywhere else. (On ARM, we pass in an invalid temp reg that is never used.)

So I updated the patch to expose what's going on with a comment, but it's pretty ugly, and I'm not sure if you meant me to just give ARM a proper temporary and be done with it? I think that would work, though it would slightly reduce the quality of generated code, and I don't know what's customary here.

Rather than spending time on this, I probably could have learned about CacheIR or something and done a proper fix. But before I throw this back on my ignore pile, I guess I'd like to get feedback just for my own education. Thanks!

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8972649 [details] [diff] [review]
Remove extractString

Review of attachment 8972649 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. Looks good, I think this should work on all platforms with the suggested changes below.

(The SharedIC changes should be temporary until Matthew converts that to CacheIR.)

::: js/src/jit/BaselineIC.cpp
@@ +3209,5 @@
>          masm.loadValue(sepAddr, sepVal);
>          masm.branchTestString(Assembler::NotEqual, sepVal, &failureRestoreArgc);
>  
> +        Register sep = sepVal.scratchReg();
> +        masm.unboxNonDouble(sepVal, sep, JSVAL_TYPE_STRING);

Nit: could use masm.unboxString(val, reg) here and elsewhere in the patch.

::: js/src/jit/CodeGenerator.cpp
@@ +7663,5 @@
>      masm.bind(&string);
> +    // If we have a temp register, use it. Currently, only ARM32 won't have it
> +    // but the payload is in a register already so unboxNonDouble(leftV, left)
> +    // is a no-op (other than Spectre protection.)
> +    Register left = tempToUnbox == Register::Invalid() ? leftV.payloadReg() : tempToUnbox;

Two things:

* It's not just ARM32 but all 32-bit platforms (x86, MIPS32).
* leftV.payloadReg() does not compile on 64-bit platforms.

Maybe the simplest and most straight-forward way is this:

#if defined(JS_NUNBOX32)
    Register left = leftV.payloadReg();
#else
    Register left = tempToUnbox;
#endif
    masm.unboxString(leftV, left);
    ...

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f63e6668a23
Remove extractString, r=jandem

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/0f63e6668a23