Bug 122939 – Baseline JIT and DFG IC code generation should be unified and rationalized

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Currently there's a bunch of stuff that assumes that an IC corresponds to a byte code index and a machine code return address within the machine code of a CodeBlock.

It's fine that IC's are owned by a Codeblocks.  But we want to allow for IC's to be created in an ad-hoc manner.

It's fine that an IC is part of the machine code stream of a CodeBlock.  But we want to allow them to be created out-of-line, also.

[Filip Pizlo]

Ideally, you'd be able to say something like:

ICGenerator ic(codeBlock, codeOrigin, identifier, type, registersToUse);

ic.generateFastPath(&jit);
ic.generateSlowPath(&jit);
ic.finalize(linkBuffer);
ic.populateStubInfo(stubInfo);

And then be able to call into the fast path and it should work as you'd expect.

This will allow us to further unify baseline and DFG IC code.  And the FTL will be able to just use this, modulo a custom link buffer that would generate the code into the llvm patch point.

[Filip Pizlo]

Created attachment 214634 [details]
it begins

[Filip Pizlo]

Created attachment 214636 [details]
it's sort of starting to make sense

[Filip Pizlo]

Created attachment 214639 [details]
it's growing

[Filip Pizlo]

Created attachment 214658 [details]
it compiles!

And it crashes on *every* test.  It's an impressive achievement!

[Filip Pizlo]

Created attachment 214664 [details]
the patch

[WebKit Commit Bot]

Attachment 214664 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/assembler/AbstractMacroAssembler.h', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/dfg/DFGInlineCacheWrapper.h', u'Source/JavaScriptCore/dfg/DFGInlineCacheWrapperInlines.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/jit/GPRInfo.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp', u'Source/JavaScriptCore/jit/JITInlineCacheGenerator.h', u'Source/JavaScriptCore/jit/JITPropertyAccess.cpp', u'Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp', u'Source/JavaScriptCore/jit/RegisterSet.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGInlineCacheWrapperInlines.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/dfg/DFGInlineCacheWrapper.h:47:  The parameter name "linkBuffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:58:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:109:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:109:  The parameter name "codeOrigin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:110:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:111:  The parameter name "ecmaMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:111:  The parameter name "putKind" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 214666 [details]
the patch

Fix some stuff.

[WebKit Commit Bot]

Attachment 214666 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/assembler/AbstractMacroAssembler.h', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/dfg/DFGInlineCacheWrapper.h', u'Source/JavaScriptCore/dfg/DFGInlineCacheWrapperInlines.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/jit/GPRInfo.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp', u'Source/JavaScriptCore/jit/JITInlineCacheGenerator.h', u'Source/JavaScriptCore/jit/JITPropertyAccess.cpp', u'Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp', u'Source/JavaScriptCore/jit/RegisterSet.h']" exit_code: 1
Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:58:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:110:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 214666 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214666&action=review

r=me

> Source/JavaScriptCore/bytecode/CodeBlock.h:150
>      bool isStrictMode() const { return m_isStrictMode; }
> +    ECMAMode ecmaMode() const { return isStrictMode() ? StrictMode : NotStrictMode; }

Can we just change m_isStrictMode to m_ecmaMode?

> Source/JavaScriptCore/dfg/DFGInlineCacheWrapper.h:49
> +    GeneratorType m_gen;

m_generator?

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:229
> +    for (unsigned i = m_getByIds.size(); i--;)
> +        m_getByIds[i].finalize(linkBuffer);
> +    for (unsigned i = m_putByIds.size(); i--;)
> +        m_putByIds[i].finalize(linkBuffer);

So. Much. Better.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:369
> +    ECMAMode strictModeFor(CodeOrigin codeOrigin)

ecmaModeFor?

[Filip Pizlo]

(In reply to comment #10)
> (From update of attachment 214666 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214666&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/bytecode/CodeBlock.h:150
> >      bool isStrictMode() const { return m_isStrictMode; }
> > +    ECMAMode ecmaMode() const { return isStrictMode() ? StrictMode : NotStrictMode; }
> 
> Can we just change m_isStrictMode to m_ecmaMode?

I'll leave the CodeBlock changes for later.  A boolean does have one virtue: it's a byte while an enum will be an int.

> 
> > Source/JavaScriptCore/dfg/DFGInlineCacheWrapper.h:49
> > +    GeneratorType m_gen;
> 
> m_generator?

OK.

> 
> > Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:229
> > +    for (unsigned i = m_getByIds.size(); i--;)
> > +        m_getByIds[i].finalize(linkBuffer);
> > +    for (unsigned i = m_putByIds.size(); i--;)
> > +        m_putByIds[i].finalize(linkBuffer);
> 
> So. Much. Better.
> 
> > Source/JavaScriptCore/jit/AssemblyHelpers.h:369
> > +    ECMAMode strictModeFor(CodeOrigin codeOrigin)
> 
> ecmaModeFor?

OK.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/157685

[Mark Hahnenberg]

> > Can we just change m_isStrictMode to m_ecmaMode?
> 
> I'll leave the CodeBlock changes for later.  A boolean does have one virtue: it's a byte while an enum will be an int.
> 
You can force enums to be a byte! (in C++11)

enum MyEnum : unsigned char {
// blah blah blah
};