1479603 - Inline Caching Cleanup

Status: RESOLVED FIXED

Description

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

As more pieces of the IC system get converted to CacheIR, there's a number opportunity to generally cleanup the Inline Caching story: 

* There's a good number of functions in the wrong files (ie, things in SharedIC.h that are only ever referred to in CacheIRCompiler files). 
* Once Bugs 1478126 and 1341261 land some IC Machinery (ICMultiStubCompiler) goes dead. 

There's also an opportunity to split apart SharedIC.h into some files with some more structure. 

I was thinking something like: 

ICCommon.{h,cpp}:   Basic IC definitions; ICEntry, ICStub{Const}Iterator, 
                    ICStub, ICFallback Stub
TypeIC.{h,cpp}:     The type IC mechanisms; TypeCheckPrimitiveSetStub and 
                    subclasses. 
SharedIC.{h,cpp}:   Kept as homes for things using the Shared stubs machinery;
                    ICNewObject_Fallback etc.

Comment 1

[Jan de Mooij [:jandem]]

Eventually the shared stubs machinery should just be removed. Once it's only used for ICNew{Object,Array} that's probably not enough justification to keep all that code. IIRC these New* stubs were ported to shared stubs "because we can" and I'm not sure having them in Ion is really necessary for perf; I might be wrong on this, worth checking the original bugs that made them shared stubs.

So then everything in {Baseline,Shared}IC.{h,cpp} will be Baseline specific and I like using BaselineIC.{h,cpp} for them (also for consistency with IonIC.{h,cpp}). We could still split the Type Monitor/Update stubs out to BaselineTypeIC.{h,cpp} or something.

WDYT?

Comment 2

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

I definitely like the idea of keeping baseline-with-baseline in files named Baseline. 

I do still a common base file may make sense (though, I haven't done the work to tease it out yet, so could be more tightly woven than I'd like)

I definitely would love to get rid of the last vestiges of the SharedIC stuff, but an experimental try push [1] with just the NewArray and NewObject Ion ICs disabled suggest that on some workloads they're still providing value ("tp6_youtube opt 1_thread e10s stylo" shows a pretty big degradation; though, "tp6_youtube opt e10s stylo" does not -- curious). 

If we don't get rid of it, I'd still say segregating them into SharedIC.{cpp,h} makes sense (at least to draw a target on their back for CacheIR conversion (planning on investigating that later today anyhow). 


[1]: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=ddf5d6bd6e6c5f9dfe638e0e413bcad458f854f0&framework=1&selectedTimeRange=172800

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #2)
> I do still a common base file may make sense (though, I haven't done the
> work to tease it out yet, so could be more tightly woven than I'd like)

But common in what sense? If Baseline and Ion, their IC code is basically unshared except for the CacheIR code. If BaselineIC and SharedIC, I think that's not worth the trouble if SharedIC is dying soon anyway.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 1] Cleanup IC code as we show SharedICs the door

- Move SimpleTypeDescrKey functions into CacheIR.h. This matches the set of callers
- Move LoadTypedThingData into CacheIRCompiler, matching usage
- Push definition of StripPreliminaryObjectStubs up in file, and remove header declaration
- Consolidate LoadTypedThingLength into CacheIR files
- Remove now dead ICMultiStubCompiler
- Remove BinarySharedStub classes. Can do this now that Compare and BinaryArith ICs have been converted to CacheIR
- Rename comapreSharedStub, and use correct flag to disable it.
- Remove SharedStubInfo
- Collapse SharedICList.h back into SharedIC.h
- Remove unimplemented declaration

(This used to be a bunch of patches, but I folded them for review ease; can unfold if desired)

Comment 5

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 2] Cleanup Ion SharedStubs code

Comment 6

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 3] Remove Shared Stubs option

Comment 7

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

PS: I haven't written this patch yet, but what do you think about renaming the last of the 'shared stubs' machinery to 'fallback stub' machinery. (I am thinking specifically of [1] and [2])  

[1]: https://searchfox.org/mozilla-central/rev/5dbfd833bbb114afe758db4d4bdbc5b13bcc33ef/js/src/jit/CodeGenerator.cpp#10393 
[2]: https://searchfox.org/mozilla-central/rev/5dbfd833bbb114afe758db4d4bdbc5b13bcc33ef/js/src/jit/CodeGenerator.h#293-303

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 9001475 [details] [diff] [review]
[Part 1] Cleanup IC code as we show SharedICs the door

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

::: js/src/jit/BaselineIC.cpp
@@ +547,5 @@
>  
>      return tailCallVM(DoToNumberFallbackInfo, masm);
>  }
>  
> +void

Nice, we can now use "static void" here because it's only used in BaselineIC.cpp right?

::: js/src/jit/CacheIRCompiler.h
@@ +905,5 @@
> +void
> +LoadTypedThingData(MacroAssembler& masm, TypedThingLayout layout, Register obj, Register result);
> +
> +void
> +LoadTypedThingLength(MacroAssembler& masm, TypedThingLayout layout, Register obj, Register result);

Maybe these two could now be protected methods (loadTypedThingData, loadTypedThingLength) of the CacheIRCompiler class. Follow-up patch? :)

::: js/src/jit/SharedIC.h
@@ +193,5 @@
>  
>  class ICStub;
>  class ICFallbackStub;
>  
> +// List of IC stub kinds that can run in Baseline and in IonMonkey

This list can now be merged with BaselineICList right?

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 9001476 [details] [diff] [review]
[Part 2] Cleanup Ion SharedStubs code

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

\o/

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 9001477 [details] [diff] [review]
[Part 3] Remove Shared Stubs option

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

We can also remove this option from this test file:

jit-test/tests/basic/write-frozen-dense-strict-inlinecache.js

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #7)
> PS: I haven't written this patch yet, but what do you think about renaming
> the last of the 'shared stubs' machinery to 'fallback stub' machinery. (I am
> thinking specifically of [1] and [2])  

I think on top of your other patches, this is all dead code :) We can also remove ICStubEngine::IonSharedIC then (and probably remove ICStubEngine from a lot of places where it's used).

Comment 12

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 4] Remove SharedIC support from Ion

Comment 13

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Comment on attachment 9001723 [details] [diff] [review]
[Part 4] Remove SharedIC support from Ion

You're 100% correct. :D

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 9001723 [details] [diff] [review]
[Part 4] Remove SharedIC support from Ion

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

::: js/src/jit/CodeGenerator.cpp
@@ -10561,5 @@
> -    // Patch shared stub IC loads using IC entries
> -    for (size_t i = 0; i < sharedStubs_.length(); i++) {
> -        CodeOffset label = sharedStubs_[i].label;
> -
> -        IonICEntry& entry = ionScript->sharedStubList()[i];

IonICEntry can also be removed now and we can then merge ICEntry and BaselineICEntry \o/

::: js/src/jit/IonCode.h
@@ +545,5 @@
>          recompiling_ = false;
>      }
>  
>      FallbackICStubSpace* fallbackStubSpace() {
>          return &fallbackStubSpace_;

All this can also go away.

Comment 16

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 1 Fixup] Rename SHARED_STUB_KIND_LIST to CACHEIR_STUB_KIND_LIST

Once the old shared stub kind list is folded back into the baseline list where
appropriate, that leaves behind the CacheIR stub kinds, which don't feel like they
belong.

So I was thinking I'd keep them separate, but name them as such.

Comment 17

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 5] Relocate Baseline fallback stubs to BaselineIC.h

Comment 18

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 6] Reduce redundant usage of ICStubEngine

Comment 19

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 7] Collapse IonICEntry and BaselineICEntry into ICEntry

Comment 20

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 8] Remove Ion Fallback stub space

Comment 21

[Jan de Mooij [:jandem]]

Comment on attachment 9002453 [details] [diff] [review]
[Part 1 Fixup] Rename SHARED_STUB_KIND_LIST to CACHEIR_STUB_KIND_LIST

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

::: js/src/jit/SharedIC.h
@@ +193,5 @@
>  
>  class ICStub;
>  class ICFallbackStub;
>  
> +// Cache IR stubs can be generated for both Baseline and IonMonkey

CacheIR works for both Baseline and Ion ICs, but this is the Baseline specific part and these stub kinds are only used for Baseline. I think we should just add them to the Baseline list.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 9002513 [details] [diff] [review]
[Part 6] Reduce redundant usage of ICStubEngine

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

::: js/src/jit/BaselineIC.h
@@ +542,5 @@
>          }
>  
>        public:
> +        explicit Compiler(JSContext* cx, bool hasReceiver = false)
> +          : ICStubCompiler(cx, ICStub::GetProp_Fallback, Engine::Baseline),

We can also remove the engine field from ICStubCompiler :)

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 9002514 [details] [diff] [review]
[Part 7] Collapse IonICEntry and BaselineICEntry into ICEntry

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

Excellent.

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 9002515 [details] [diff] [review]
[Part 8] Remove Ion Fallback stub space

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

::: js/src/jit/SharedIC.h
@@ +1109,5 @@
>  
>    public:
>      virtual ICStub* getStub(ICStubSpace* space) = 0;
>  
>      static ICStubSpace* StubSpaceForStub(bool makesGCCalls, JSScript* outerScript, Engine engine) {

We can remove the engine argument here too :)

Comment 25

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 9] Remove ICStubCompiler::Engine

Comment 26

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 10] Relocate the remaining Baseline-only stub code to BaselineIC files

Comment 27

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

(this cleanup is broader and farther reaching than I had foreseen!)

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 9002783 [details] [diff] [review]
[Part 9] Remove ICStubCompiler::Engine

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

Nice, r=me with nits addressed.

::: js/src/jit/BaselineIC.h
@@ +302,5 @@
>          bool hasReceiver_;
>          MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override;
>  
>          virtual int32_t getKey() const override {
> +            return static_cast<int32_t>(kind) << 1 |

We can remove the |<< 1| now and change the << 17 below to << 16

@@ +534,5 @@
>          MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override;
>          void postGenerateStubCode(MacroAssembler& masm, Handle<JitCode*> code) override;
>  
>          virtual int32_t getKey() const override {
> +            return static_cast<int32_t>(kind) << 1 |

And here.

@@ +666,5 @@
>          MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override;
>          void postGenerateStubCode(MacroAssembler& masm, Handle<JitCode*> code) override;
>  
>          virtual int32_t getKey() const override {
> +            return static_cast<int32_t>(kind) << 1 |

And here.

@@ +755,5 @@
>      uint32_t pcOffset_;
>      MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override;
>  
>      virtual int32_t getKey() const override {
> +        return static_cast<int32_t>(kind) << 1 |

Same here (also please keep the parentheses around the <<)

@@ +852,5 @@
>          uint32_t pcOffset_;
>          MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override;
>  
>          virtual int32_t getKey() const override {
> +            return static_cast<int32_t>(kind) << 1 |

And here.

@@ +932,5 @@
>          uint32_t pcOffset_;
>          MOZ_MUST_USE bool generateStubCode(MacroAssembler& masm) override;
>  
>          virtual int32_t getKey() const override {
> +            return static_cast<int32_t>(kind) << 1 |

And here.

::: js/src/jit/SharedIC.cpp
@@ -536,5 @@
> -    if (engine_ == Engine::Baseline) {
> -        EmitBaselineTailCallVM(code, masm, argSize);
> -    } else {
> -        uint32_t stackSize = argSize + fun.extraValuesToPop * sizeof(Value);
> -        EmitIonTailCallVM(code, masm, stackSize);

EmitIonTailCallVM, more dead code :)

@@ +584,5 @@
>  ICStubCompiler::leaveStubFrame(MacroAssembler& masm, bool calledIntoIon)
>  {
>      MOZ_ASSERT(entersStubFrame_ && inStubFrame_);
>      inStubFrame_ = false;
> +    

Remove trailing whitespace.

::: js/src/jit/SharedIC.h
@@ +1011,5 @@
>  #endif
>  
>      // By default the stubcode key is just the kind.
>      virtual int32_t getKey() const {
> +        return (static_cast<int32_t>(kind) << 1);

return static_cast<int32_t>(kind);

@@ +1195,5 @@
>          TypeCheckPrimitiveSetStub* existingStub_;
>          uint16_t flags_;
>  
>          virtual int32_t getKey() const override {
> +            return static_cast<int32_t>(kind) << 1 |

Can remove the << 1 and s/17/16/

Comment 29

[Jan de Mooij [:jandem]]

Comment on attachment 9002784 [details] [diff] [review]
[Part 10] Relocate the remaining Baseline-only stub code to BaselineIC files

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

::: js/src/jit/SharedIC.h
@@ -688,4 @@
>      // that these do not get purged, all stubs that can make calls are allocated
>      // in the fallback stub space.
>      bool allocatedInFallbackSpace() const {
>          MOZ_ASSERT(next());

I think all of this other code (ICStub etc) is also Baseline specific.

Comment 30

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 11] Remove SharedIC.h,cpp

Woo! You're totally right.

I attempted to verify the new headers with a FILES_PER_UNIFIED_FILE = 1 local build, and this
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42e47dda9873d02aadbbb44f19c8cb549ae42761

Hopefully that'll be enough to avoid breaking non-unified builds.

Comment 31

[Matthew Gaudet (he/him) [:mgaudet] (on parental leave until September 3)]

Attachment: [Part 12] Simplify getKey computations

Sorry for the extra patch, but I had some trouble trying to get this change into the stack with minimal fixups, so it was just much easier to toss it on top. Appreciate it :)

Comment 32

[Jan de Mooij [:jandem]]

Comment on attachment 9003167 [details] [diff] [review]
[Part 11] Remove SharedIC.h,cpp

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

\o/ This is awesome.

::: js/src/jit/BaselineIC.h
@@ +8,5 @@
>  #define jit_BaselineIC_h
>  
>  #include "mozilla/Assertions.h"
>  
> +

Nit: remove the extra blank line

::: js/src/jit/CacheIR.h
@@ +13,5 @@
>  
>  #include "gc/Rooting.h"
>  #include "jit/CompactBuffer.h"
>  #include "jit/ICState.h"
> +#include "jit/MacroAssembler.h"

Does it compile without this #include? I don't think CacheIR.h itself depends on masm.

Comment 33

[Jan de Mooij [:jandem]]

Comment on attachment 9003241 [details] [diff] [review]
[Part 12] Simplify getKey computations

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

Thanks!

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7518b4bbc6c7
[Part 1] Cleanup IC code as we show SharedICs the door. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/402c68b550d1
[Part 2] Cleanup Ion SharedStubs code r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a64325eeacb
[Part 3] Remove Shared Stubs option r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/92b4cab82784
[Part 4] Remove SharedIC support from Ion r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/68b7d42b931d
[Part 5] Relocate Baseline fallback stubs to BaselineIC.h r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bdec5f4de88
[Part 6] Reduce redundant usage of ICStubEngine r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/652d157c6c62
[Part 7] Collapse IonICEntry and BaselineICEntry into ICEntry r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/c594e6a34377
[Part 8] Remove Ion Fallback stub space r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/b733b2488363
[Part 9] Remove ICStubCompiler::Engine r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/370a45e8ea65
[Part 10] Relocate the remaining Baseline-only stub code to BaselineIC files r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a2381311ced
[Part 11] Remove SharedIC.h,cpp r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0c2d31df1e
[Part 12] Simplify getKey computations r=jandem

Comment 35

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7518b4bbc6c7
https://hg.mozilla.org/mozilla-central/rev/402c68b550d1
https://hg.mozilla.org/mozilla-central/rev/9a64325eeacb
https://hg.mozilla.org/mozilla-central/rev/92b4cab82784
https://hg.mozilla.org/mozilla-central/rev/68b7d42b931d
https://hg.mozilla.org/mozilla-central/rev/4bdec5f4de88
https://hg.mozilla.org/mozilla-central/rev/652d157c6c62
https://hg.mozilla.org/mozilla-central/rev/c594e6a34377
https://hg.mozilla.org/mozilla-central/rev/b733b2488363
https://hg.mozilla.org/mozilla-central/rev/370a45e8ea65
https://hg.mozilla.org/mozilla-central/rev/5a2381311ced
https://hg.mozilla.org/mozilla-central/rev/1d0c2d31df1e