Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: mgaudet, Assigned: mgaudet)

Tracking

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(12 attachments, 1 obsolete attachment)

29.12 KB, patch
jandem
: review+
Details | Diff | Splinter Review
10.77 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.88 KB, patch
jandem
: review+
Details | Diff | Splinter Review
18.09 KB, patch
jandem
: review+
Details | Diff | Splinter Review
7.26 KB, patch
jandem
: review+
Details | Diff | Splinter Review
46.49 KB, patch
jandem
: review+
Details | Diff | Splinter Review
29.12 KB, patch
jandem
: review+
Details | Diff | Splinter Review
8.45 KB, patch
jandem
: review+
Details | Diff | Splinter Review
61.08 KB, patch
jandem
: review+
Details | Diff | Splinter Review
108.40 KB, patch
jandem
: review+
Details | Diff | Splinter Review
71.02 KB, patch
jandem
: review+
Details | Diff | Splinter Review
7.59 KB, patch
jandem
: review+
Details | Diff | Splinter Review
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.
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?
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
(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.
Priority: -- → P3
- 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)
Attachment #9001475 - Flags: review?(jdemooij)
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 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?
Attachment #9001475 - Flags: review?(jdemooij) → review+
Comment on attachment 9001476 [details] [diff] [review]
[Part 2] Cleanup Ion SharedStubs code

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

\o/
Attachment #9001476 - Flags: review?(jdemooij) → review+
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
Attachment #9001477 - Flags: review?(jdemooij) → review+
(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 on attachment 9001723 [details] [diff] [review]
[Part 4] Remove SharedIC support from Ion

You're 100% correct. :D
Attachment #9001723 - Flags: review?(jdemooij)
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.
Attachment #9001723 - Flags: review?(jdemooij) → review+
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.
Attachment #9002453 - Flags: review?(jdemooij)
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.
Attachment #9002453 - Flags: review?(jdemooij)
Attachment #9002512 - Flags: review?(jdemooij) → review+
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 :)
Attachment #9002513 - Flags: review?(jdemooij) → review+
Comment on attachment 9002514 [details] [diff] [review]
[Part 7] Collapse IonICEntry and BaselineICEntry into ICEntry

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

Excellent.
Attachment #9002514 - Flags: review?(jdemooij) → review+
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 :)
Attachment #9002515 - Flags: review?(jdemooij) → review+
(this cleanup is broader and farther reaching than I had foreseen!)
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/
Attachment #9002783 - Flags: review?(jdemooij) → review+
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.
Attachment #9002784 - Flags: review?(jdemooij) → review+
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.
Attachment #9003167 - Flags: review?(jdemooij)
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 :)
Attachment #9003241 - Flags: review?(jdemooij)
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.
Attachment #9003167 - Flags: review?(jdemooij) → review+
Comment on attachment 9003241 [details] [diff] [review]
[Part 12] Simplify getKey computations

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

Thanks!
Attachment #9003241 - Flags: review?(jdemooij) → review+
Pushed by mgaudet@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.