Closed Bug 1341261 Opened 3 years ago Closed Last year

Port Compare IC to CacheIR

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox56 --- fixed
firefox63 --- fixed

People

(Reporter: evilpie, Assigned: mgaudet)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [qf-])

Attachments

(18 files, 25 obsolete files)

2.84 KB, patch
jandem
: review+
Details | Diff | Splinter Review
4.27 KB, patch
jandem
: review+
Details | Diff | Splinter Review
4.37 KB, patch
jandem
: review+
Details | Diff | Splinter Review
5.36 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.75 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
2.72 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
2.97 KB, patch
mgaudet
: review+
Details | Diff | Splinter Review
12.09 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
16.43 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
6.59 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
1.00 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
12.09 KB, patch
decoder
: feedback+
Details | Diff | Splinter Review
6.79 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
5.04 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
11.99 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
10.65 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
6.84 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
62.78 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
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.
Whiteboard: [qf-]
I agree on that!
Priority: -- → P3
Assignee: nobody → evilpies
Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch WIP v2 (obsolete) — Splinter Review
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?
Attachment #8876869 - Attachment is obsolete: true
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.
Attachment #8882198 - Flags: review?(jdemooij)
Attachment #8882234 - Flags: review?(jdemooij)
Attachment #8882234 - Attachment description: v2 CompareIR spewing → v1 CompareIR spewing
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.
Attachment #8882248 - Flags: review?(jdemooij)
Attachment #8882248 - Attachment description: v2 - 1st Stub: Strings → v1 - 1st Stub: Strings
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.
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 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 :)
Attachment #8882198 - Flags: review?(jdemooij) → review+
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.
Attachment #8882234 - Flags: review?(jdemooij) → review+
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.
Attachment #8882248 - Flags: review?(jdemooij) → review+
Attachment #8879711 - Attachment is obsolete: true
(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.
Flags: needinfo?(jdemooij)
(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.
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8218ddede57b
Use CompareIR for string equality. r=jandem
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
Flags: needinfo?(evilpies)
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.
Flags: needinfo?(jdemooij)
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.
Flags: needinfo?(evilpies)
I changed the patch so that instead of adding a PointerOperandId we use two different CacheIR opcodes.
Attachment #8882281 - Attachment is obsolete: true
Attachment #8885409 - Flags: review?(jdemooij)
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Comment on attachment 8885409 [details] [diff] [review]
v2 - 2nd Stub: Symbols and Objects

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

Great.
Attachment #8885409 - Flags: review?(jdemooij) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63215af4995f
Use CompareIR for symbol and object equality. r=jandem
(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!
Depends on: 1412527
Assignee: evilpies → mgaudet
Status: REOPENED → ASSIGNED
Original implementation by evilpie, this patch just adds a testcase.
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
Attachment #8952837 - Flags: review?(tcampbell)
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.
Attachment #8952837 - Flags: review?(tcampbell)
Attachment #8952837 - Attachment is obsolete: true
This op allows extracting the type tag for future uses. This commit introduces
ValueTypeOperandId, which can be used to provide type safety around tags.
Attachment #8953413 - Flags: review?(tcampbell)
Attachment #8953413 - Attachment is obsolete: true
Attachment #8953413 - Flags: review?(tcampbell)
This op allows extracting the type tag for future uses. This commit introduces
ValueTypeOperandId, which can be used to provide type safety around tags.
Attachment #8953420 - Flags: review?(tcampbell)
Does a tag compare on two types; fails for number tags
Attachment #8953421 - Flags: review?(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 :)
Attachment #8953420 - Flags: review?(tcampbell) → review+
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.
Attachment #8953421 - Flags: review?(tcampbell) → review+
Comment on attachment 8953422 [details] [diff] [review]
[Part 3] Inline cache for comparing strictly different types

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

Excellent!
Attachment #8953422 - Flags: review?(tcampbell) → review+
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!
This op allows extracting the type tag for future uses. This commit introduces
ValueTagOperandId, which can be used to provide type safety around tags.
Attachment #8953420 - Attachment is obsolete: true
Does a tag compare on two types; fails for number tags
Attachment #8953421 - Attachment is obsolete: true
Attachment #8953422 - Attachment is obsolete: true
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.
Attachment #8954206 - Flags: review+
Attachment #8954207 - Flags: review+
Attachment #8954208 - Flags: review+
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.
Pushed by mgaudet@mozilla.com:
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
Depends on: 1438727
As a bonus, also cleanup a superflous trackAttached
Note: This produces potentially more stubs than the SharedIC implmentation it
is intended to replace, however, the generated stubs are simpler.
Blocks: 1467907
Depends on: 1472233
As a bonus, also cleanup a superflous trackAttached
Attachment #8994927 - Flags: review?(tcampbell)
Attachment #8984578 - Attachment is obsolete: true
Attachment #8984579 - Attachment is obsolete: true
Attachment #8984580 - Attachment is obsolete: true
Attachment #8984581 - Attachment is obsolete: true
Attachment #8984582 - Attachment is obsolete: true
Attachment #8984583 - Attachment is obsolete: true
Attachment #8984584 - Attachment is obsolete: true
Note: This produces potentially more stubs than the SharedIC implmentation it
is intended to replace, however, the generated stubs are simpler.
Attachment #8994929 - Flags: review?(tcampbell)
Also, relocate no-longer SharedIC machinery to BaselineIC.cpp
Attachment #8994932 - Flags: review?(tcampbell)
Attachment #8994941 - Flags: feedback?(nth10sd)
Attachment #8994941 - Flags: feedback?(choller)
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.
Attachment #8994941 - Attachment is obsolete: true
Attachment #8994941 - Flags: feedback?(nth10sd)
Attachment #8994941 - Flags: feedback?(choller)
Attachment #8995166 - Flags: feedback?(nth10sd)
Attachment #8995166 - Flags: feedback?(choller)
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.
Attachment #8995166 - Flags: feedback?(nth10sd) → feedback+
Blocks: 1479603
Attachment #8882337 - Attachment is obsolete: true
Attachment #8882363 - Attachment is obsolete: true
Attachment #8994925 - Flags: review?(tcampbell) → review+
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.
Attachment #8994926 - Flags: review?(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.
Attachment #8994927 - Flags: review?(tcampbell) → review+
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.
Attachment #8994928 - Flags: review?(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.
Attachment #8994929 - Flags: review?(tcampbell)
Attachment #8994933 - Flags: review?(tcampbell) → review+
Comment on attachment 8995166 [details] [diff] [review]
Rollup For Fuzzing Apply To 12f42d129762

Didn't see any crashes within 24 hours.
Attachment #8995166 - Flags: feedback?(choller) → feedback+
Attachment #8994928 - Attachment is obsolete: true
Attachment #8994929 - Attachment is obsolete: true
Attachment #8994926 - Attachment is obsolete: true
Attachment #8997498 - Flags: review?(tcampbell)
Attachment #8997497 - Flags: review?(tcampbell) → review+
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
Attachment #8998883 - Flags: review?(tcampbell)
Attachment #8997498 - Attachment is obsolete: true
Attachment #8997498 - Flags: review?(tcampbell)
Attachment #8994931 - Attachment is obsolete: true
Attachment #8994931 - Flags: review?(tcampbell)
Attachment #8999183 - Attachment is obsolete: true
Attachment #8999183 - Flags: review?(tcampbell)
Comment on attachment 8999237 [details] [diff] [review]
[Part 9] Rename CacheRegisterAllocator::loadDouble to ensureDoubleRegister

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

Excellent
Attachment #8999237 - Flags: review?(tcampbell) → review+
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);
Attachment #8998883 - Flags: review?(tcampbell) → review+
Attachment #8997499 - Flags: review?(tcampbell) → review+
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?
Attachment #8998964 - Flags: review?(tcampbell) → review+
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.
Attachment #8994932 - Flags: review?(tcampbell)
Attachment #8994924 - Flags: review?(tcampbell) → review+
Also, relocate no-longer SharedIC machinery to BaselineIC.cpp
Attachment #8999283 - Flags: review?(tcampbell)
Attachment #8994932 - Attachment is obsolete: true
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).
Attachment #8999283 - Flags: review?(tcampbell) → review+
Keywords: leave-open
Pushed by mgaudet@mozilla.com:
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
Depends on: 1482906
Depends on: 1483189
Blocks: 1483340
Depends on: 1483542
Depends on: 1503116
You need to log in before you can comment on or make changes to this bug.