Bug 122939

Summary:

Baseline JIT and DFG IC code generation should be unified and rationalized

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

atrick, barraclough, commit-queue, ggaren, gyuyoung.kim, mark.lam, mhahnenberg, msaboff, nrotem, oliver, rakuco, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

122940

Bug Blocks:

122739

Attachments:

Description	Flags
it begins	none
it's sort of starting to make sense	none
it's growing	none
it compiles!	none
the patch	none
the patch	ggaren: review+

Filip Pizlo

Reported 2013-10-16 21:25:01 PDT

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.

Attachments
it begins (8.91 KB, patch) 2013-10-18 21:22 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
it's sort of starting to make sense (25.26 KB, patch) 2013-10-18 22:14 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
it's growing (44.19 KB, patch) 2013-10-18 23:06 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
it compiles! (57.57 KB, patch) 2013-10-19 11:56 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (78.97 KB, patch) 2013-10-19 13:21 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (79.32 KB, patch) 2013-10-19 13:28 PDT, Filip Pizlo	ggaren: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2013-10-16 22:48:41 PDT

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

Comment 2 2013-10-18 21:22:07 PDT

Created attachment 214634 [details] it begins

Filip Pizlo

Comment 3 2013-10-18 22:14:32 PDT

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

Filip Pizlo

Comment 4 2013-10-18 23:06:58 PDT

Created attachment 214639 [details] it's growing

Filip Pizlo

Comment 5 2013-10-19 11:56:23 PDT

Created attachment 214658 [details] it compiles! And it crashes on *every* test. It's an impressive achievement!

Filip Pizlo

Comment 6 2013-10-19 13:21:47 PDT

Created attachment 214664 [details] the patch

WebKit Commit Bot

Comment 7 2013-10-19 13:23:52 PDT

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

Comment 8 2013-10-19 13:28:24 PDT

Created attachment 214666 [details] the patch Fix some stuff.

WebKit Commit Bot

Comment 9 2013-10-19 13:29:32 PDT

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 10 2013-10-19 14:34:39 PDT

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

Comment 11 2013-10-19 14:42:23 PDT

(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

Comment 12 2013-10-19 16:12:02 PDT

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

Mark Hahnenberg

Comment 13 2013-10-21 10:09:46 PDT

> > 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 };

Note You need to log in before you can comment on or make changes to this bug.