Closed Bug 1432168 Opened 2 years ago Closed 2 years ago

Convert ToBool Inline Cache to CacheIR

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mgaudet, Assigned: mgaudet)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 7 obsolete files)

3.71 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
27.37 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
2.44 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
The ToBool inline cache can be converted to CacheIR.
Assignee: nobody → mgaudet
Comment on attachment 8944543 [details] [diff] [review]
Convert ToBool inline cache to CacheIR

Looking for a bit of feedback on this patch. One particular stand out curiosity is the distinction between the guardType(..., JSVAL_TYPE_DOUBLE), and (what I've implmented as) guardDouble. It's clear these two mean different things... but I don't think I know what the distinction _is_.
Attachment #8944543 - Flags: feedback?(jdemooij)
Comment on attachment 8944543 [details] [diff] [review]
Convert ToBool inline cache to CacheIR

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

Nice! The more we convert the better.

::: js/src/jit/BaselineIC.h
@@ -242,4 @@
>      };
>  };
>  
> -class ICToBool_Int32 : public ICStub

You can also remove these stubs from BaselineICList.h (maybe also the InstanceOf one you replaced? I don't remember).

::: js/src/jit/CacheIR.cpp
@@ +4649,5 @@
> +    //
> +    //     writer.guardType(valId, JSVAL_TYPE_DOUBLE);
> +    //
> +    //         However, the above uses testNumber, rather than
> +    //         the expected testDouble.

Maybe we should add guardNumber and change the behavior of guardType?

@@ +4665,5 @@
> +        return false;
> +
> +    ValOperandId valId(writer.setInputOperandId(0));
> +    writer.guardIsString(valId);
> +    writer.loadStringTruthyResult(valId);

I think it would be nice to do this:

  StringOperandId strId = writer.guardIsString(valId);
  writer.loadStringTruthyResult(strId);

Then we don't have to unbox the Value a second time in loadStringTruthyResult and we can just compare the string length to 0.

We could add guardIsInt32 etc and do something similar there.
Attachment #8944543 - Flags: feedback?(jdemooij) → feedback+
Blocks: CacheIR
So, it seems that currently, branchTestStringTruthy takes a value operand. It seems to me, that if we try to do the unboxing earlier, that's going to cause some scope creep by requiring the implementation of an unboxed variant of branchTestStringTruthy, along with the macro assembler machinery that implements that. 

Perhaps that should be a follow-up item?
(In reply to Matthew Gaudet (he/him) [:mgaudet] (UTC-5) from comment #5)
> So, it seems that currently, branchTestStringTruthy takes a value operand.
> It seems to me, that if we try to do the unboxing earlier, that's going to
> cause some scope creep by requiring the implementation of an unboxed variant
> of branchTestStringTruthy, along with the macro assembler machinery that
> implements that. 

You could just do

  masm.branch32(Assembler::Equal, Address(str, JSString::offsetOfLength()), Imm32(0), &isFalse);
To make this change, replace calls to guardType that currently have DOUBLE as the value
with ones that use a new guardNumber, which tests for Numbers instead.
Attachment #8945798 - Attachment is obsolete: true
Attachment #8945798 - Flags: review?(jdemooij)
Attachment #8945798 - Attachment is obsolete: false
Attachment #8945798 - Flags: review?(jdemooij)
Attachment #8945799 - Attachment is obsolete: true
Attachment #8945799 - Flags: review?(jdemooij)
Comment on attachment 8945798 [details] [diff] [review]
[Part 1]: Make guardType(..., JSVAL_TYPE_DOUBLE) check for Double, and not Number

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

Looks good!

::: js/src/jit/CacheIR.cpp
@@ +4029,4 @@
>      if (!val_.isPrimitive())
>          return false;
>  
> +    if (val_.isNumber()) {

Nit: no {} for this if-else since each of condition/then-body/else-body fit on a single line.

::: js/src/jit/CacheIRCompiler.cpp
@@ +1201,5 @@
>  bool
> +CacheIRCompiler::emitGuardIsNumber()
> +{
> +    ValOperandId inputId = reader.valOperandId();
> +    if (allocator.knownType(inputId) == JSVAL_TYPE_DOUBLE) // Doubles are numbers!

integers are numbers too, so you could do something like:

JSValueType knownType = allocator.knownType(inputId);
if (knownType == JSVAL_TYPE_INT32 || knownType == JSVAL_TYPE_DOUBLE)
    return true;
Attachment #8945798 - Flags: review?(jdemooij) → review+
To make this change, replace calls to guardType that currently have DOUBLE as the value
with ones that use a new guardNumber, which tests for Numbers instead.
Comment on attachment 8946366 [details] [diff] [review]
[Part 1]: Make guardType(..., JSVAL_TYPE_DOUBLE) check for Double, and not Number

Carrying r+ from jandem -- nits addressed.
Attachment #8946366 - Flags: review+
Attachment #8945809 - Attachment is obsolete: true
Attachment #8945809 - Flags: review?(jdemooij)
Comment on attachment 8946371 [details] [diff] [review]
[Part 2] Convert ToBool inline cache to CacheIR

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

Beautiful!

::: js/src/jit-test/tests/cacheir/tobool.js
@@ +8,5 @@
> +
> +function test_type_stable_ic() {
> +    // Initialize as falsy where possible.
> +    var a = undefined; // No Change, never succeeds
> +    var b = null;      // No Change, never succeeeds

Nit: succeeds

@@ +12,5 @@
> +    var b = null;      // No Change, never succeeeds
> +    var c = false;     // Alternate between true and false.
> +    var d = 0;         // Int32 cache checker, change int values
> +    var e = "";        // String cache checker, change string values
> +    var f = Symbol();  // No change, always succeed, no cache.

It's so easy to support this (guardType + return true) that we should probably just spend the 2 minutes on it. Follow-up bug/patch?

::: js/src/jit/CacheIR.cpp
@@ +4672,5 @@
> +
> +bool
> +ToBoolIRGenerator::tryAttachNullOrUndefined()
> +{
> +    if (!val_.isNull() && !val_.isUndefined())

Nit: using !val_.isNullOrUndefined() may be nice since it matches "NullOrUndefined" in tryAttachNullOrUndefined :)

::: js/src/jit/CacheIRCompiler.cpp
@@ +2312,5 @@
> +{
> +    AutoOutputRegister output(*this);
> +    ValueOperand val = allocator.useValueRegister(masm, reader.valOperandId());
> +
> +    Label ifFalse,done;

Nit: space after , and below
Attachment #8946371 - Flags: review?(jdemooij) → review+
To make this change, replace calls to guardType that currently have DOUBLE as the value
with ones that use a new guardNumber, which tests for Numbers instead.
Comment on attachment 8946784 [details] [diff] [review]
[Part 3]: Add ToBool IC for Symbols

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

Yay, IC coverage!

::: js/src/jit-test/tests/cacheir/tobool.js
@@ +89,5 @@
>  test_transition(fun5, {}, null, 1, 0);
> +test_transition(fun6, null, {},  0, 1);
> +// Symbol -> null, null -> Symbol
> +test_transition(fun5, Symbol('hi'), null, 1, 0);
> +test_transition(fun6, null, Symbol('lo'),  0, 1);
\ No newline at end of file

Did you intend to reuse fun5/fun6? I'm fine with it either way.
Attachment #8946784 - Flags: review?(tcampbell) → review+
I am still sad each time I see the document.all / objectEmulatesUndefined stuff in the spec..
(In reply to Ted Campbell [:tcampbell] from comment #19)
> Did you intend to reuse fun5/fun6? I'm fine with it either way.

No I did not! Good catch, thanks. 

(In reply to Ted Campbell [:tcampbell] from comment #20)
> I am still sad each time I see the document.all / objectEmulatesUndefined
> stuff in the spec..

That was a confusing diversion, so I guess I am sad too :P
Comment on attachment 8946878 [details] [diff] [review]
[Part 3]: Add ToBool IC for Symbols

Carrying tcampbell's review after addressing function recycling nit.
Attachment #8946878 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75fec9a94607
Convert ToBool inline cache to CacheIR. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bdf289f4ef
Add ToBool IC for Symbols. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cba4fe27807
Make guardType(..., JSVAL_TYPE_DOUBLE) check for Double, and not Number. r=tcampbell
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.