1695396 - Move isFirstStub from CallIRGenerator into IRGenerator

Status: RESOLVED FIXED

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

Description

[Iain Ireland [:iain]]

Note: this bug is currently reserved for Outreachy applicants for the Spring/Summer 2021 cycle. If it has not been completed by the end of the application cycle, we will open it up.

Background

Inline caches (ICs) are an important technique for optimizing JS. When we execute code that has many possible behaviours (for example, a + b might be addition or string concatenation, depending on what a and b are), we call a fallback function that tries to attach an IC stub. (For example, it might check whether a and b are numbers, and immediately return their sum if they are.) The next time we execute that code, we will try the stub first, and only execute the fallback function if the stub fails. If the code is polymorphic, we may attach several stubs to the same IC chain.

There are many different kinds of IC, each of which has its own implementation of IRGenerator.

This Bug

When deciding what kind of stub to attach, one thing that we might want to consider is whether we've already attached a stub, or whether this is the first time we've reached this code. If we reach the fallback after attaching a stub, we may choose to be more conservative for the next stub.

We have already implemented this for Call ICs. However, it would be useful if other IRGenerators could also check whether this is the first IC we've attached. (For example, it would have been handy here in bug 1693328.)

The purpose of this bug is to move isFirstStub_ out of CallIRGenerator into IRGenerator, the shared superclass. This will require changing uses of the other subclasses of IRGenerator to supply the required information.

(Note that isFirstStub is determined by looking at the ICState on the fallback stub, and the ICState::Mode argument to the IRGenerator constructor also comes from the ICState. It may make sense to pass in a single reference to the ICState, instead of two separate arguments.)

Once you have moved isFirstStub into IRGenerator, you can also change this code in OptimizeSpreadCallIRGenerator::tryAttachArray to use isFirstStub instead.

When you're done, you should be able to run the test suite (./mach jit-test) without any failures. (You will want to build SpiderMonkey with --enable-optimize --enable-debug while running the tests.)

Prerequisites

Before getting started, you'll want to ensure:

• you have a checkout of the Firefox source code
• you can build SpiderMonkey
• you have read this walkthrough about how development works in Firefox

Getting help

Leave comments on this bug for questions, or ask in #spidermonkey on chat.mozilla.org.

Comment 1

[mehaboob shariff]

can I get this bug

Comment 2

[Iain Ireland [:iain]]

Yep! Go right ahead!

Comment 3

[mehaboob shariff]

thank you

Comment 4

[mehaboob shariff]

In the last part where it is asked to change the code in tryAttachArray but when I click "this code" part it is pointing me to a different part of the code

Comment 5

[mehaboob shariff]

Attachment: Bug 1695396: Moved isFirstStub_ into IRGenerator in CacheIR.h r=iain

Comment 6

[mehaboob shariff]

Thank you iain for reviewing the bug. I was a bit confused whether to change the subclass in other files apart from the header file but when I tested it it showed passed all cases thats why I pushed it.
Now when I ran the tests it seems to be failing so I will make the changes.

So to make the changes to the patch I can hg commit --amend and change the code again right?

And one more doubt do I have to include the CacheIR header file in Baseline.cpp and IonIC.cpp ?

Comment 7

[Iain Ireland [:iain]]

(In reply to mehaboob shariff from comment #6)

Thank you iain for reviewing the bug. I was a bit confused whether to change the subclass in other files apart from the header file but when I tested it it showed passed all cases thats why I pushed it.
Now when I ran the tests it seems to be failing so I will make the changes.

Oh, I bet I know what happened. If you changed the header file and rebuilt, but didn't notice that the build failed. The build process only replaces your old build if it succeeds, so if you run the tests after a failed build you will use your last successful build, which doesn't include your changes.

So to make the changes to the patch I can hg commit --amend and change the code again right?

If you start with your current patch and make your changes, then once you are happy with them, hg commit --amend will combine them with your existing patch.

And one more doubt do I have to include the CacheIR header file in Baseline.cpp and IonIC.cpp ?

It's already included indirectly. For example, BaselineIC.cpp includes BaselineCacheIRCompiler.h, which includes CacheIR.h. (It's not always easy to tell whether a header is included. It's usually okay to assume that .cpp files already include all the necessary headers, and let the compiler point out cases where that's not true.)

Comment 8

[mehaboob shariff]

I have made a submission can I get a review

Comment 9

[mehaboob shariff]

Thanks for reviewing Iain
In the second patch I was trying to experiment a bit. I thought that why do we do need to initialize everywhere, I just needed to use isFirstStub to modify a part of OptimizedIRGenerator so I just passed the value in it.
Thanks again, this review has cleared a lot of doubts.

Comment 10

[Iain Ireland [:iain]]

Another good way to tell that your patch works is that you will be able to implement this part:

Once you have moved isFirstStub into IRGenerator, you can also change this code in OptimizeSpreadCallIRGenerator::tryAttachArray to use isFirstStub instead.

Comment 11

[mehaboob shariff]

Yeah I have changed that part in tryattacharray . Just a few errors left related to "TryAttach" part and will solve that also. I will be off for 1 or 2 days and look at the rest of errors after I'm home.
Sorry for the late and inconvenience.

Comment 12

[mehaboob shariff]

I have looked at it again but it shows error at only GetPropIRGenerator even though I have passed the arguments. There is no problem with rest of the constructors.

 0:13.52 mozmake.EXE[4]: *** Waiting for unfinished jobs....
 0:13.57 In file included from Unified_cpp_js_src_jit1.cpp:38:
 0:13.57 c:/mozilla-source/mozilla-unified/js/src/jit/BaselineIC.cpp(659,17): error: no matching constructor for initialization of 'js::jit::GetPropIRGenerator'
 0:13.58     IRGenerator gen(cx, script, pc, stub->state().mode(), isFirstStub,
 0:13.58                 ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:13.58 c:/mozilla-source/mozilla-unified/js/src/jit/BaselineIC.cpp(881,3): note: in instantiation of function template specialization 'js::jit::TryAttachStub<js::jit::GetPropIRGenerator, bool &, js::jit::CacheKind, JS::Handle<JS::Value> &, JS::Handle<JS::Value> &>' requested here
 0:13.58   TryAttachStub<GetPropIRGenerator>("GetElem", cx, frame, stub, isFirstStub,
 0:13.58   ^
 0:13.58 c:/mozilla-source/mozilla-unified/js/src\jit/CacheIR.h(1379,3): note: candidate constructor not viable: requires 8 arguments, but 9 were provided
 0:13.58   GetPropIRGenerator(JSContext* cx, HandleScript script, jsbytecode* pc,
 0:13.58   ^
 0:13.58 c:/mozilla-source/mozilla-unified/js/src\jit/CacheIR.h(1271,16): note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 9 were provided
 0:13.58 class MOZ_RAII GetPropIRGenerator : public IRGenerator {
 0:13.58                ^
 0:13.59 In file included from Unified_cpp_js_src_jit1.cpp:38:
 0:13.59 c:/mozilla-source/mozilla-unified/js/src/jit/BaselineIC.cpp(659,17): error: no matching constructor for initialization of 'js::jit::GetPropIRGenerator'
 0:13.59     IRGenerator gen(cx, script, pc, stub->state().mode(), isFirstStub,
 0:13.59                 ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:13.59 c:/mozilla-source/mozilla-unified/js/src/jit/BaselineIC.cpp(1478,3): note: in instantiation of function template specialization 'js::jit::TryAttachStub<js::jit::GetPropIRGenerator, bool &, js::jit::CacheKind, JS::MutableHandle<JS::Value> &, JS::Rooted<JS::Value> &>' requested here
 0:13.59   TryAttachStub<GetPropIRGenerator>("GetProp", cx, frame, stub, isFirstStub,
 0:13.59   ^
 0:13.59 c:/mozilla-source/mozilla-unified/js/src\jit/CacheIR.h(1379,3): note: candidate constructor not viable: requires 8 arguments, but 9 were provided
 0:13.60   GetPropIRGenerator(JSContext* cx, HandleScript script, jsbytecode* pc,
 0:13.60   ^
 0:13.60 c:/mozilla-source/mozilla-unified/js/src\jit/CacheIR.h(1271,16): note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 9 were provided
 0:13.60 class MOZ_RAII GetPropIRGenerator : public IRGenerator 

Comment 13

[Iain Ireland [:iain]]

It looks like you're accidentally passing too many arguments on lines 881 and 1478 of BaselineIC.cpp. (Look for "note: candidate constructor not viable: requires 8 arguments, but 9 were provided".)

Comment 14

[mehaboob shariff]

Yes, I have made the corrections and made a submission.
Can you please check if there are still more changes that need to be made

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f89eb149d04
Moved isFirstStub_ into IRGenerator in CacheIR.h r=iain

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/4f89eb149d04