1499644 - Move IC data out of BaselineScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

ICEntries and the fallback stub space could be moved into a separate class that's stored in JSScript.

This is necessary for bug 1499324, but it will also allow us to simplify some BaseilneDebugModeOSR code (in particular, the DebugModeOSRVolatileStub complexity can go away I think) and that makes this worth doing now IMO.

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> This is necessary for bug 1499324, but it will also allow us to simplify
> some BaseilneDebugModeOSR code (in particular, the DebugModeOSRVolatileStub
> complexity can go away I think)

Also the IC stub cloning we will no longer need! That's a bit less boilerplate with CacheIR, but it's still code we should be able to remove.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: WIP

This is a quick-and-dirty WIP patch:

* Adds ICScript storing the fallback stub space and the ICEntries.

* Removes > 230 lines of code from BaselineDebugModeOSR.* (and also related code from CacheIR and BaselineIC.cpp) because we no longer need to clone stubs.

* Passes almost all jit-tests, except for one known issue: ICTableSwitch needs to be rewritten (bug 1501316), the patch tries to workaround it but it doesn't work when we compile multiple BaselineScripts for one ICScript.

* The patch also never destroys ICScript once it's created. Shouln't be hard to fix.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: WIP

(See the previous comment.)

Ted, do you mind glancing at this for a minute or two, to see if the approach makes sense to you? There are a bunch of known issues/nits, I'm just requesting feedback for the overall design.

Comment 4

[Jan de Mooij [:jandem]]

diffstat, FWIW:

 19 files changed, 660 insertions(+), 869 deletions(-)

Comment 5

[Ted Campbell [:tcampbell]]

Comment on attachment 9019680 [details] [diff] [review]
WIP

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

ICScript seems like the right approach. I think this direction is worth continuing.

Leaving f? so I remember to give it more thought about details this week.

::: js/src/jit/BaselineIC.cpp
@@ +123,5 @@
>      }
>  }
>  
> +/* static */ bool
> +ICScript::create(JSContext* cx, JSScript* script)

We need a better way to correlate these. Some sort of JOF_* or equivalent would be nice so we can better assert on all side.

::: js/src/vm/JSScript.h
@@ +1415,5 @@
>    private:
>      /* Persistent type information retained across GCs. */
>      js::TypeScript* types_ = nullptr;
>  
> +    js::jit::ICScript* icScript_ = nullptr;

Unifying this with TypeScript one day would be nice. We probably have similar criteria for deciding this script is worth tracking types and adding ICs too. It also tells a nice story for Ion inspection.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Ted Campbell [:tcampbell] from comment #5)
> We need a better way to correlate these. Some sort of JOF_* or equivalent
> would be nice so we can better assert on all side.

Yeah, I think JOF_IC would be really nice to have (and we probably need it anyway, longer term). That said, the current patch will complain loudly if either (a) you add an IC to ICScript but don't use it in BaselineCompiler, or (b) you *don't* add an IC but expect one in BaselineCompiler. So I'd prefer adding JOF_IC in a follow-up bug (right after this lands) to not blow this patch up more.

> Unifying this with TypeScript one day would be nice. We probably have
> similar criteria for deciding this script is worth tracking types and adding
> ICs too. It also tells a nice story for Ion inspection.

Long-term I want to consider removing TypeScript completely and storing the StackTypeSets directly in the type monitor stubs (that will also save some memory because we allocate type monitor stubs lazily for JOF_TYPESET ops!). After that we could also remove StackTypeSet completely and having Ion build its TemporaryTypeSets directly from the attached type monitor stubs :)

Comment 7

[Ted Campbell [:tcampbell]]

(In reply to Jan de Mooij [:jandem] from comment #6) 
> Long-term I want to consider removing TypeScript completely and storing the
> StackTypeSets directly in the type monitor stubs (that will also save some
> memory because we allocate type monitor stubs lazily for JOF_TYPESET ops!).
> After that we could also remove StackTypeSet completely and having Ion build
> its TemporaryTypeSets directly from the attached type monitor stubs :)

A+

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 9019680 [details] [diff] [review]
WIP

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

::: js/src/jit/x64/SharedICHelpers-x64.h
@@ +35,3 @@
>  {
> +    // Move ICEntry* into ICStubReg.
> +    masm.movePtr(ImmPtr(entry), ICStubReg);

Note to self: now that ICEntry* is a constant known at compile time we could probably use loadPtr + AbsoluteAddress... Doesn't matter much on x64 but on platforms like x86 it can eliminate an instruction.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 9019680 [details] [diff] [review]
WIP

I'll try to rebase and finish this soon. The updated patch should be nicer/simpler on top of the patches in the dependent bugs.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: WIP v2

Rebased + some cleanup.

On top of all the changes that landed in the dependent bugs this now passes all jit-tests with --baseline-eager.

This needs a bunch of work still before we can land it, but it's moving in the right direction.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: WIP v3

This version should be green on Try on top of the patches in the dependent bugs.

Now we just need some minor clean up work but everything should be in place.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1499644 - Move IC data out of BaselineScript. r?tcampbell

ICEntries and the fallback stub space are now stored in ICScript. The ICScript*
is stored in TypeScript to not increase sizeof(JSScript).

We need this for bug 1499324 but it also lets us greatly simplify the
BaselineDebugModeOSR code as this patch shows.

Note: some ICScript method definitions are still in BaselineJIT.cpp instead of
BaselineIC.cpp to make this patch easier to review. We could move them to
BaselineIC.cpp as a follow-up change.

Comment 13

[Jan de Mooij [:jandem]]

Ted, if you want to play with this locally, here's a Try push that has the most recent version except for some comments I added later: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=5290d02778215bf80b44cca98eadf5bdc1c7d094

Also this still removes ~250 lines of code:

 30 files changed, 721 insertions(+), 962 deletions(-)

The patch is big but a lot of it is very mechanical changes.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6453222232be
Move IC data out of BaselineScript. r=tcampbell

Comment 15

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/6453222232be