1168750 - [MBaselineCache] Refactoring files

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

Before doing anything that affects how IonMonkey and Baseline works, I want to do the needed file moves. These changes should impose little to no logical changes. (These are also the changes that will bitrot the fastest)

Like:
BaselineHelper-xxx -> JitStubHelper-xxx.
BaselineIC -> SharedIC, BaselineIC, IC.
ICStubCompiler::getKey to include engine field.
BaselineStubReg -> ICStubReg
...

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Part1: rename baselinehelpers/registers to jitstub

Renames some Baseline specific files to JitStub. Since those will be shared with both.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Part2: rename BaselineTailCallReg / BaselineStubReg

Both these registers are not specific to baseline anymore. But are inherent to the stub infrastructure

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Part3: split BaselineIC and add IC and SharedIC

IC.h contains all logic for the system
SharedIC.h will contain all stubs that work on IonMonkey and on Baseline
BaselineIC. will contain the stubs that work only in Baseline

As a result I had to add BaselineICList and SharedICList which contain a list of stubs that IC.h can use.

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Part 4: add awareness of different engines

Last in a list of patches that will bitrot quite fast. If possible don't let the reviews take too long. I made a separation between these and the next. Since these are quite easy and don't change a lot to the logical part. The upcoming patches won't bitrot that easily and will contain the actual logic.

Added a field to ICStubCompiler to know if it is compiling for ionmonkey or baseline. This is needed since tailVMCall/tailCall/pushBaselineFramePtr/... will behave differently depending on the engine.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8611242 [details] [diff] [review]
Part1: rename baselinehelpers/registers to jitstub

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

This looks good, but can we do s/JitStub/SharedIC/? I think SharedIC is a bit less ambiguous and it'd nicely match the SharedIC.{cpp,h} added in another patch.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8611244 [details] [diff] [review]
Part2: rename BaselineTailCallReg / BaselineStubReg

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

::: js/src/jit/x86/JitStubRegisters-x86.h
@@ +24,3 @@
>  // registers from R2.
> +static MOZ_CONSTEXPR_VAR Register JitTailCallReg      = esi;
> +static MOZ_CONSTEXPR_VAR Register JitStubReg          = edi;

I'd prefer ICTailCallReg, ICStubReg, it's a bit more specific.

(Or SharedICTailCallReg, SharedICStubReg but I think that's a bit too long and less readable.)

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8611245 [details] [diff] [review]
Part3: split BaselineIC and add IC and SharedIC

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

As discussed on IRC, I think we should merge BaselineIC.cpp/h and IC.cpp/h for now, to avoid confusion with IonCaches.cpp/h

Also, can you do |hg cp BaselineIC.cpp SharedIC.cpp| (and same for the .h), and then remove the lines we don't want in BaselineIC.* and SharedIC.*? That way hg blame still works.

::: js/src/moz.build
@@ +200,5 @@
>      'jit/EagerSimdUnbox.cpp',
>      'jit/EdgeCaseAnalysis.cpp',
>      'jit/EffectiveAddressAnalysis.cpp',
>      'jit/ExecutableAllocator.cpp',
> +    'jit/IC.cpp',

We should also add SharedIC.cpp.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8611248 [details] [diff] [review]
Part 4: add awareness of different engines

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

Looks good. The getKey code is a bit repetitive but that's a pre-existing problem (I have some plans to refactor it, but doesn't need to happen here).

::: js/src/jit/IC.h
@@ +949,5 @@
>      // Prevent GC in the middle of stub compilation.
>      js::gc::AutoSuppressGC suppressGC;
>  
> +  public:
> +    enum Engine {

Can you make this |enum class Engine| and then use Engine::Foo instead of ICStubCompiler::Foo in most places?

@@ +958,4 @@
>    protected:
>      JSContext* cx;
>      ICStub::Kind kind;
> +    Engine engine;

Maybe engine_? cx/kind don't have an underscore, but entersStubFrame_ below has and most subclasses of ICStubCompiler also use this convention.

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Part1: rename baselinehelpers/registers to SharedIC

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: 8611245: Part3: split BaselineIC and add SharedIC

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8615365 [details] [diff] [review]
8611245: Part3: split BaselineIC and add SharedIC

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

Great, thanks!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c37863f4faa4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b827cbcab49
https://hg.mozilla.org/integration/mozilla-inbound/rev/728b11ef982d
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d79aafe62a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c37863f4faa4
https://hg.mozilla.org/mozilla-central/rev/7b827cbcab49
https://hg.mozilla.org/mozilla-central/rev/728b11ef982d
https://hg.mozilla.org/mozilla-central/rev/02d79aafe62a