1432168 - Convert ToBool Inline Cache to CacheIR

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

The ToBool inline cache can be converted to CacheIR.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Convert ToBool inline cache to CacheIR

Comment 2

[Matthew Gaudet (he/him) [: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_.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

(Try push on patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=167b8203eb49a176f9f3a6802a08613f66595658)

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

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?

Comment 6

[Jan de Mooij [:jandem]]

(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);

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 1]: Make guardType(..., JSVAL_TYPE_DOUBLE) check for Double, and not Number

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 8

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 2]: Convert ToBool inline cache to CacheIR

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 2]: Convert ToBool inline cache to CacheIR

Comment 10

[Jan de Mooij [:jandem]]

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;

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 1]: Make guardType(..., JSVAL_TYPE_DOUBLE) check for Double, and not Number

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 12

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 13

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 2] Convert ToBool inline cache to CacheIR

Comment 14

[Jan de Mooij [:jandem]]

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

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 1]: Make guardType(..., JSVAL_TYPE_DOUBLE) check for Double, and not Number

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 16

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 2]: Convert ToBool inline cache to CacheIR

Carrying r+ with nits addressed.

Comment 17

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 3]: Add ToBool IC for Symbols

Comment 18

[Matthew Gaudet (he/him) [:mgaudet]]

Try Build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49c311e23fb00c0218e2f5dd7673780680e533bb&selectedJob=159360627

Comment 19

[Ted Campbell [:tcampbell]]

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.

Comment 20

[Ted Campbell [:tcampbell]]

I am still sad each time I see the document.all / objectEmulatesUndefined stuff in the spec..

Comment 21

[Matthew Gaudet (he/him) [:mgaudet]]

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

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 3]: Add ToBool IC for Symbols

Comment 23

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8946878 [details] [diff] [review]
[Part 3]: Add ToBool IC for Symbols

Carrying tcampbell's review after addressing function recycling nit.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/75fec9a94607
https://hg.mozilla.org/mozilla-central/rev/f7bdf289f4ef
https://hg.mozilla.org/mozilla-central/rev/8cba4fe27807