1341261 - Port Compare IC to CacheIR

Status: RESOLVED FIXED

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

Description

[Tom S. (please needinfo tschuster)]

The SharedIC code for CompareICs is pretty complex and actually doesn't seem to handle everything that is interesting. Using CacheIR for this should make the code cleaner and the analyzer should point out missing cases.

Comment 1

[Hannes Verschore [:h4writer]]

I agree on that!

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: WIP

This is just the basic infrastructure to support this IC with CacheIR in baseline. I want to come up with some way to allow a wider range of comparisons, like null <> boolean or boolean <> int. Adding a strict equality stub for different types (ie. "kind" === true, is always false) would be interesting for one Speedometer benchmark.

Comment 3

[Tom S. (please needinfo tschuster)]

Attachment: WIP v2

A bit more progresss. The int32 stubs seems to be highly optimized at the moment, it's going to be interesting to see the difference in generated code. For double comparisons we seem to require platform specific code, not sure yet how to implement this, maybe a new MacroAssembler method?

There is some things that aren't really that nice: Symbols and objects are just pointer compares, but should we use ValOperandId for those, or introduce something new?

Comment 4

[Tom S. (please needinfo tschuster)]

Attachment: v1 CompareIR basic outline

I kind of want to land this patch set piece by piece, i.e add about one or two stubs per patch. This only supports Baseline, because we can only enable this for Ion after everything is implemented.

Comment 5

[Tom S. (please needinfo tschuster)]

Attachment: v1 CompareIR spewing

Comment 6

[Tom S. (please needinfo tschuster)]

Attachment: v1 - 1st Stub: Strings

This is the first stub that only works with equality: string comparison. There is still room for improvement here, we should compare string chars inline (bug 1326193) and have some kind of fallback VM call.

Comment 7

[Tom S. (please needinfo tschuster)]

Attachment: v1 - 2nd Stub: Symbols and Objects

Comment 8

[Tom S. (please needinfo tschuster)]

Attachment: v1 - 3rd Stub: Strict equality with different types

So this patch is still a bit rough, but the basic idea for this is to optimize the case where we have different types and strict equality. This always fails and we get the benefit of not having to handle this anywhere else.

Comment 9

[Tom S. (please needinfo tschuster)]

Attachment: v1 - 4th Stubs: int/boolean/null comparisons

With this patch we full support {int, boolean, null} x {int, boolean, null}, where x are all the (relational) comparisons operators. As far as I understand only strict equality of course should not work across different types, and null is not actually equal to int or boolean, but still works for <, <= etc. This relies on the payload of null having a zero bit pattern.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8882198 [details] [diff] [review]
v1 CompareIR basic outline

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

Sorry for the delay. Thanks for working on this!

::: js/src/jit/SharedIC.cpp
@@ +1456,4 @@
>          return true;
>      }
>  
> +    if (engine == Engine::Baseline) {

This means we can't (incrementally) remove the stubs we convert, but that seems okay. Would be very cool to use "real" Ion ICs for this + arithmetic ICs later so we can remove the shared stubs code :)

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8882234 [details] [diff] [review]
v1 CompareIR spewing

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

::: js/src/jit/CacheIR.cpp
@@ +3729,5 @@
> +    if (sp.enabled()) {
> +        LockGuard<Mutex> guard(sp.lock());
> +        sp.beginCache(guard, *this);
> +        sp.valueProperty(guard, "base", lhsVal_);
> +        sp.valueProperty(guard, "property", rhsVal_);

Can we rename "base" and "property" to "lhs" and "rhs" or does that confuse the spew reader? Also below.

::: js/src/jit/SharedIC.cpp
@@ +1456,4 @@
>          return true;
>      }
>  
> +    if (engine == ICStubCompiler::Engine::Baseline) {

ICStubEngine::Baseline should also work.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8882248 [details] [diff] [review]
v1 - 1st Stub: Strings

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

Nice.

::: js/src/jit/CacheIR.cpp
@@ +3713,5 @@
> +        return false;
> +
> +    StringOperandId lhsStrId = writer.guardIsString(lhsId);
> +    StringOperandId rhsStrId = writer.guardIsString(rhsId);
> +    writer.compareStringResult(op_, lhsStrId, rhsStrId);

Note that baking in the JSOp means we can't use the same JIT code for (for instance) JSOP_EQ and JSOP_STRICTEQ even though the code is exactly the same.

This is also true for the current compare stubs and CacheIR stubs are shared by all compartments within a Zone, so this patch is a strict improvement anyway.

The easiest fix would be to use a bool or to always pass JSOP_STRICTEQ/JSOP_STRICTNE, but we can also just leave it like this.

Comment 13

[Tom S. (please needinfo tschuster)]

(In reply to Jan de Mooij [:jandem] from comment #10)
> Comment on attachment 8882198 [details] [diff] [review]
> v1 CompareIR basic outline
> 
> Review of attachment 8882198 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay. Thanks for working on this!
> 
> ::: js/src/jit/SharedIC.cpp
> @@ +1456,4 @@
> >          return true;
> >      }
> >  
> > +    if (engine == Engine::Baseline) {
> 
> This means we can't (incrementally) remove the stubs we convert, but that
> seems okay. Would be very cool to use "real" Ion ICs for this + arithmetic
> ICs later so we can remove the shared stubs code :)

Thanks for reviewing. Maybe you can give the other patches a quick look as well? I am especially curious about your opinion on the int/bool patch, because we have all that specialized code for int32 compares in Baseline.
Sadly replacing the IC system in Ion is a lot harder, and supporting an intermediate state is not worth the effort.

Comment 14

[Tom S. (please needinfo tschuster)]

(In reply to Jan de Mooij [:jandem] from comment #11)
> Comment on attachment 8882234 [details] [diff] [review]
> v1 CompareIR spewing
> 
> Review of attachment 8882234 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/CacheIR.cpp
> @@ +3729,5 @@
> > +    if (sp.enabled()) {
> > +        LockGuard<Mutex> guard(sp.lock());
> > +        sp.beginCache(guard, *this);
> > +        sp.valueProperty(guard, "base", lhsVal_);
> > +        sp.valueProperty(guard, "property", rhsVal_);
> 
> Can we rename "base" and "property" to "lhs" and "rhs" or does that confuse
> the spew reader? Also below.
> 
Yeah it does, but I am probably going to special handling for compare to the CacheIR analyzer instead. The current behavior isn't very useful and this would probably work for arithmetic stubs as well.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f86880276d
Basic framework for CompareIR code. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca7dfedd992
CompareIR spewing. r=jandem

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8218ddede57b
Use CompareIR for string equality. r=jandem

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequently failing spidermonkey cgc's js/src/jit-test/tests/asm.js/testBug1117235.js on Linux x64 debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/78c399bd23b6c95d3c9a13d3a7fef5e84b144024

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8218ddede57b853db1a420db4957f229ef229805&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=112789409&repo=mozilla-inbound

[task 2017-07-08T18:14:58.484645Z] Exit code: -9
[task 2017-07-08T18:14:58.484716Z] TIMEOUT - asm.js/testBug1117235.js
[task 2017-07-08T18:14:58.484856Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/asm.js/testBug1117235.js | Timeout (code -9, args "--baseline-eager")
[task 2017-07-08T18:14:58.484893Z] INFO exit-status     : -9
[task 2017-07-08T18:14:58.484919Z] INFO timed-out       : True

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout of the remaining two changesets:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9623b08f4af3b12fb8fd14318cf89c13a45b54aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b47420e3a0a1243f48a8bbcb121c33e49ab809c

Comment 19

[Jan de Mooij [:jandem]]

We probably have to update BaselineInspector::expectedCompareType too...

Other patches look good to me. There are a number of different cases but each is pretty simple and the code is much nicer/cleaner than what we have now.

Maybe tryAttachIntish could be upgraded so it's able to replace tryAttachInt32/tryAttachBoolean, if it doesn't complicate the code more.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff34e592b774
Basic framework for CompareIR code. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa6c5a536141
CompareIR spewing. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/17f8bf61f6e9
Use CompareIR for string equality. r=jandem

Comment 21

[Tom S. (please needinfo tschuster)]

Based on sfink's recommendation I added the test to cgc-jittest-timeouts.txt, which stops it from running. I could even reproduce this locally on a fairly slow laptop.

Comment 22

[Tom S. (please needinfo tschuster)]

Attachment: v2 - 2nd Stub: Symbols and Objects

I changed the patch so that instead of adding a PointerOperandId we use two different CacheIR opcodes.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff34e592b774
https://hg.mozilla.org/mozilla-central/rev/aa6c5a536141
https://hg.mozilla.org/mozilla-central/rev/17f8bf61f6e9

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 8885409 [details] [diff] [review]
v2 - 2nd Stub: Symbols and Objects

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

Great.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63215af4995f
Use CompareIR for symbol and object equality. r=jandem

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63215af4995f

Comment 27

[Tom S. (please needinfo tschuster)]

(In reply to Jan de Mooij [:jandem] from comment #19)
> We probably have to update BaselineInspector::expectedCompareType too...
> 
Oh yes! We will probably going to have an intermediate period where we support both CacheIR matching and the current stuff. Also curious why we don't specialize for some other things like strings etc.

> Other patches look good to me. There are a number of different cases but
> each is pretty simple and the code is much nicer/cleaner than what we have
> now.
> 
> Maybe tryAttachIntish could be upgraded so it's able to replace
> tryAttachInt32/tryAttachBoolean, if it doesn't complicate the code more.

I looked into this again, and while it's definitely not that hard to fix, it does make the tryAttachIntish method more confusing. At least I haven't come up with a good way to express all the constraints yet. We really need to make sure that we don't accidentally start doing some unsupported comparisons!

Comment 28

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

Attachment: Handle strict equality between different types in Compare IC

Original implementation by evilpie, this patch just adds a testcase.

Comment 29

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

Comment on attachment 8952837 [details] [diff] [review]
Handle strict equality between different types in Compare IC

Tested in the browser w/ Google Docs, and saw a bunch of attached ICs. 

In a microbenchmark I've seen a >2x improvement (--no-ion)

Started a try build here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e02707f00d23a12c72495707d5c185774d5c0bb

Comment 30

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

Comment on attachment 8952837 [details] [diff] [review]
Handle strict equality between different types in Compare IC

Clearing review request; this IC isn't working for 32 bit builds, so needs some reworking.

Comment 31

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

Attachment: Implement LoadValueType in CacheIR

This op allows extracting the type tag for future uses. This commit introduces
ValueTypeOperandId, which can be used to provide type safety around tags.

Comment 32

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

Attachment: [Part 1] Implement LoadValueType in CacheIR

This op allows extracting the type tag for future uses. This commit introduces
ValueTypeOperandId, which can be used to provide type safety around tags.

Comment 33

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

Attachment: [Part 2] Implement GuardTypeNotEqual

Does a tag compare on two types; fails for number tags

Comment 34

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

Attachment: [Part 3] Inline cache for comparing strictly different types

Comment 35

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

Try build for Part 1-3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=527c2371e82b89107fef212cbac0f1815a246cd5

Comment 36

[Ted Campbell [:tcampbell]]

Comment on attachment 8953420 [details] [diff] [review]
[Part 1] Implement LoadValueType in CacheIR

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

Lets name this ValueTag instead of ValueType for clarity. See JSValueTag vs JSValueType.

Other than name, looks good :)

Comment 37

[Ted Campbell [:tcampbell]]

Comment on attachment 8953421 [details] [diff] [review]
[Part 2] Implement GuardTypeNotEqual

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

Elegant handling of the Number case :)

GuardTypeNotEqual makes sense, but change valueTypeOperand if part 1 changes.

Comment 38

[Ted Campbell [:tcampbell]]

Comment on attachment 8953422 [details] [diff] [review]
[Part 3] Inline cache for comparing strictly different types

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

Excellent!

Comment 39

[Jan de Mooij [:jandem]]

Comment on attachment 8953420 [details] [diff] [review]
[Part 1] Implement LoadValueType in CacheIR

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

::: js/src/jit/CacheIRCompiler.cpp
@@ +1671,5 @@
> +    ValueOperand val = allocator.useValueRegister(masm, reader.valOperandId());
> +    Register res = allocator.defineRegister(masm, reader.valueTypeOperandId());
> +
> +    Register tag = masm.splitTagForTest(val);
> +    masm.mov(tag, res);

Nit: splitTagForTest is a bad API (it leaks scratch registers) and this will conflict with bug 1438800.

I suggest changing it to:

Register tag = masm.extractTag(val, res);
if (tag != res)
    masm.mov(tag, res);

Looks good otherwise!

Comment 40

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

Attachment: [Part 1] Implement LoadValueTag in CacheIR

This op allows extracting the type tag for future uses. This commit introduces
ValueTagOperandId, which can be used to provide type safety around tags.

Comment 41

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

Attachment: [Part 2] Implement GuardTagNotEqual

Does a tag compare on two types; fails for number tags

Comment 42

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

Attachment: [Part 3] Inline cache for comparing strictly different types

Comment 43

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

Comment on attachment 8954206 [details] [diff] [review]
[Part 1] Implement LoadValueTag in CacheIR

Carrying tcampbell's r+ 

I took his advice to heart w.r.t naming.

Comment 44

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

Thanks to Jan about the tag note; I took his advice and stopped using splitTagForTest and started using extract tag. 

Here's my try build for this. I'm going to work through pushing this tomorrow though.

Comment 45

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

Would help if I actually linked: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b93b05374010205cc4f9b8b183da2cac3820e93a

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38f4d206e39e
[Part 1] Implement LoadValueTag in CacheIR r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/74bdd1f7a2f1
[Part 2] Implement GuardTagNotEqual r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/591a2c93a43e
[Part 3] Inline cache for comparing strictly different types r=tcampbell

Comment 47

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/38f4d206e39e
https://hg.mozilla.org/mozilla-central/rev/74bdd1f7a2f1
https://hg.mozilla.org/mozilla-central/rev/591a2c93a43e

Comment 48

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

Attachment: [Part 1] Enable CacheIR Compare ICs in Ion

Comment 49

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

Attachment: [Part 2] Support Int32/Int32 Comparisons in CacheIR

Comment 50

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

Attachment: [Part 3] Support Number/Number Comparisons in CacheIR

As a bonus, also cleanup a superflous trackAttached

Comment 51

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

Attachment: [Part 4] Support Number+Undefined Comparisons in CacheIR

Comment 52

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

Attachment: [Part 5] Compare Undefined/Null+Object

Note: This produces potentially more stubs than the SharedIC implmentation it
is intended to replace, however, the generated stubs are simpler.

Comment 53

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

Attachment: [Part 6] Teach the Baseline Inspector about the new CacheIR ops

Comment 54

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

Attachment: [Part 6] Remove now-unused SharedIC Machinery

Comment 55

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

Attachment: [Part 0] Enhance compare test case

Comment 56

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

Attachment: [Part 1] Enable CacheIR Compare ICs in Ion

Comment 57

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

Attachment: [Part 2] Support Int32/Int32 Comparisons in CacheIR

Comment 58

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

Attachment: [Part 3] Support Number/Number Comparisons in CacheIR

As a bonus, also cleanup a superflous trackAttached

Comment 59

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

Attachment: [Part 4] Support Number+Undefined Comparisons in CacheIR

Comment 60

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

Attachment: [Part 5] Compare Undefined/Null+Object

Note: This produces potentially more stubs than the SharedIC implmentation it
is intended to replace, however, the generated stubs are simpler.

Comment 61

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

Attachment: [Part 6] Teach the Baseline Inspector about the new CacheIR ops

Comment 62

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

Attachment: [Part 7] Remove now-unused SharedIC Machinery for CompareICs

Also, relocate no-longer SharedIC machinery to BaselineIC.cpp

Comment 63

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

Attachment: [Part 8] Dump opcodes for CompareIC to CacheIR logs

Comment 64

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

Attachment: BundleForFuzzing.Test.d94abfaa5992.Applies.To.12f42d129762.bundle

Comment 65

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

Attachment: Rollup For Fuzzing Apply To 12f42d129762

This is the same set of patches as in Attachment 8994941 [details]: BundleForFuzzing.Test.d94abfaa5992.Applies.To.12f42d129762.bundle but patches work better for :decoder, so obsoleting the bundle.

Comment 66

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8995166 [details] [diff] [review]
Rollup For Fuzzing Apply To 12f42d129762

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

There have been a bunch of fuzzblockers lately, but in the short amount of time I've tested, it seems to be ok.

Comment 67

[Ted Campbell [:tcampbell]]

Comment on attachment 8994926 [details] [diff] [review]
[Part 2] Support Int32/Int32 Comparisons in CacheIR

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

::: js/src/jit/CacheIR.cpp
@@ +4821,5 @@
> +    Int32OperandId rhsIntId = rhsVal_.isBoolean() ? writer.guardIsBoolean(rhsId)
> +                                                  : writer.guardIsInt32(rhsId);
> +
> +    if (lhsVal_.isInt32() != rhsVal_.isInt32() &&
> +        (op_ == JSOP_STRICTEQ || op_ == JSOP_STRICTNE))

Ideally this case is handled by CompareIRGenerator::tryAttachStrictDifferentTypes instead of duplicating code.

Comment 68

[Ted Campbell [:tcampbell]]

Comment on attachment 8994927 [details] [diff] [review]
[Part 3] Support Number/Number Comparisons in CacheIR

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

::: js/src/jit/CacheIRCompiler.cpp
@@ +2922,5 @@
> +    if (!addFailurePath(&failure))
> +        return false;
> +
> +    allocator.loadDouble(masm, reader.valOperandId(), FloatReg0);
> +    allocator.loadDouble(masm, reader.valOperandId(), FloatReg1);

Add a comment that these coerce Int32 to double under the hood. It is a little confusing that allocator.loadDouble uses ensureDouble instead of loadDouble. Perhaps allocator.loadDouble needs a rename in another bug.

Comment 69

[Ted Campbell [:tcampbell]]

Comment on attachment 8994928 [details] [diff] [review]
[Part 4] Support Number+Undefined Comparisons in CacheIR

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

Let's subsume this with the StrictEquality IC work in the earlier patches. Will require reworking the tryAttach priorities.

Comment 70

[Ted Campbell [:tcampbell]]

Comment on attachment 8994929 [details] [diff] [review]
[Part 5] Compare Undefined/Null+Object

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

We chatted about this and agreed it needs a rewrite. Problems may have been pre-existing, but now is the best time to fix it.

Comment 71

[Christian Holler (:decoder)]

Comment on attachment 8995166 [details] [diff] [review]
Rollup For Fuzzing Apply To 12f42d129762

Didn't see any crashes within 24 hours.

Comment 72

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

Attachment: [Part 2] Support Int32/Int32 Comparisons in CacheIR

Comment 73

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

Attachment: [Part 4] Compare Undefined/Null+Object

Comment 74

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

Attachment: [Part 5] Number+Null+Undefined Comparisons

Comment 75

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

And since I know Ted likes try pushes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5b1dea56d77e06036e9eee78c08dbd3771e1c90

Comment 76

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

Attachment: [Part 4] Compare Undefined/Null+Object

Updated based on some IRC feedback: We don't need to be concerned 
about ordering because the two cases are EQ/NE vs STRICTEQ/STRICTNE

Comment 77

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

Attachment: [Part 6] Teach the Baseline Inspector about the new CacheIR ops

Comment 78

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

Attachment: [Part 9] Rename CacheRegisterAllocator::loadDouble to loadDoubleRegister

Comment 79

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

Attachment: [Part 9] Rename CacheRegisterAllocator::loadDouble to ensureDoubleRegister

Comment 80

[Ted Campbell [:tcampbell]]

Comment on attachment 8999237 [details] [diff] [review]
[Part 9] Rename CacheRegisterAllocator::loadDouble to ensureDoubleRegister

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

Excellent

Comment 81

[Ted Campbell [:tcampbell]]

Comment on attachment 8998883 [details] [diff] [review]
[Part 4] Compare Undefined/Null+Object

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

Looks good.

::: js/src/jit/CacheIRCompiler.cpp
@@ +3178,5 @@
> +
> +    if (op == JSOP_STRICTEQ || op == JSOP_STRICTNE) {
> +        // obj !== undefined/null for all objects.
> +        masm.moveValue(BooleanValue(op == JSOP_STRICTNE), output.valueReg());
> +    } else {

MOZ_ASSERT(op == JSOP_EQ || op == JSOPNE);

Comment 82

[Ted Campbell [:tcampbell]]

Comment on attachment 8998964 [details] [diff] [review]
[Part 6] Teach the Baseline Inspector about the new CacheIR ops

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

*sigh*.. Ion.

This gets the job done and maybe one day we'll have a better solution for BaselineInspector.

::: js/src/jit/BaselineInspector.cpp
@@ +435,5 @@
> +//
> +// An expected parameter is provided to allow GuardType to check we read
> +// the guard from the expected operand in debug builds.
> +static bool
> +GuardType(CacheIRReader& reader, mozilla::Array<MIRType,2>& guardType)

Seems like a reasonable approach.

@@ +465,5 @@
> +            break;
> +        // 1 skip
> +        case CacheOp::GuardIsInt32:
> +            guardType[guardOperand] = MIRType::Int32;
> +            reader.skip();

// Skip over result.

@@ +469,5 @@
> +            reader.skip();
> +            break;
> +        case CacheOp::GuardIsBoolean:
> +            guardType[guardOperand] = MIRType::Boolean;
> +            reader.skip();

// Skip over result.

@@ +594,5 @@
> +        if (first_type == second_type)
> +            return first_type;
> +
> +        return CompatibleType(first_type, second_type);
> +    } else {

It is SpiderMonkey style to use:

> if (cond) {
>     ...
>     return blah;
> }
>
> ...
> return boo;

::: js/src/jit/CacheIR.cpp
@@ +4946,1 @@
>      ValOperandId lhsId(writer.setInputOperandId(0));

this should be lhsIndex, no?

Comment 83

[Ted Campbell [:tcampbell]]

Comment on attachment 8994932 [details] [diff] [review]
[Part 7] Remove now-unused SharedIC Machinery for CompareICs

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

8 files removed. Yesssss.

This seems incomplete. Shouldn't SharedICList.h be modified too?  I suspect there are a few stragglers in the Ion code as a result.

Comment 84

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

Attachment: [Part 7] Remove now-unused SharedIC Machinery for CompareICs

Also, relocate no-longer SharedIC machinery to BaselineIC.cpp

Comment 85

[Ted Campbell [:tcampbell]]

Comment on attachment 8999283 [details] [diff] [review]
[Part 7] Remove now-unused SharedIC Machinery for CompareICs

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

Great!

Looks to be pre-existing, but apparently when we use Compare ICs in Ion, we still box output boolean. Maybe open a follow-up bug. (I don't expect you to be the one to fix it).

Comment 86

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6a9544c078
[Part 0] Enhance compare test case r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/5565956d9a26
[Part 1] Enable CacheIR Compare ICs in Ion r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ede6fd95cc0
[Part 2] Support Int32/Int32 Comparisons in CacheIR r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/24cdf8f1b8af
[Part 3] Support Number/Number Comparisons in CacheIR r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3455cc95ec0
[Part 4] Compare Undefined/Null+Object r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d21ae32eca7
[Part 5] Number+Null+Undefined Comparisons r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4d6c397f94c
[Part 6] Teach the Baseline Inspector about the new CacheIR ops r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/c65164fbc41c
[Part 7] Remove now-unused SharedIC Machinery for CompareICs r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/b712adf814a3
[Part 8] Dump opcodes for CompareIC to CacheIR logs r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f962180068b
[Part 9] Rename CacheRegisterAllocator::loadDouble to ensureDoubleRegister r=tcampbell

Comment 87

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/3f6a9544c078
https://hg.mozilla.org/mozilla-central/rev/5565956d9a26
https://hg.mozilla.org/mozilla-central/rev/0ede6fd95cc0
https://hg.mozilla.org/mozilla-central/rev/24cdf8f1b8af
https://hg.mozilla.org/mozilla-central/rev/b3455cc95ec0
https://hg.mozilla.org/mozilla-central/rev/2d21ae32eca7
https://hg.mozilla.org/mozilla-central/rev/b4d6c397f94c
https://hg.mozilla.org/mozilla-central/rev/c65164fbc41c
https://hg.mozilla.org/mozilla-central/rev/b712adf814a3
https://hg.mozilla.org/mozilla-central/rev/2f962180068b