Bug 122939

Summary: Baseline JIT and DFG IC code generation should be unified and rationalized
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: 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: P2    
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+

Description Filip Pizlo 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.
Comment 1 Filip Pizlo 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.
Comment 2 Filip Pizlo 2013-10-18 21:22:07 PDT 
Created attachment 214634 [details]
it begins
Comment 3 Filip Pizlo 2013-10-18 22:14:32 PDT 
Created attachment 214636 [details]
it's sort of starting to make sense
Comment 4 Filip Pizlo 2013-10-18 23:06:58 PDT 
Created attachment 214639 [details]
it's growing
Comment 5 Filip Pizlo 2013-10-19 11:56:23 PDT 
Created attachment 214658 [details]
it compiles!

And it crashes on *every* test.  It's an impressive achievement!
Comment 6 Filip Pizlo 2013-10-19 13:21:47 PDT 
Created attachment 214664 [details]
the patch
Comment 7 WebKit Commit Bot 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.
Comment 8 Filip Pizlo 2013-10-19 13:28:24 PDT 
Created attachment 214666 [details]
the patch

Fix some stuff.
Comment 9 WebKit Commit Bot 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.
Comment 10 Geoffrey Garen 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?
Comment 11 Filip Pizlo 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.
Comment 12 Filip Pizlo 2013-10-19 16:12:02 PDT 
Landed in http://trac.webkit.org/changeset/157685
Comment 13 Mark Hahnenberg 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
};