Bug 177922 – Refactor the inliner to simplify block linking

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

[Robin Morisset]

This bug is to discuss separately the refactoring I started in https://bugs.webkit.org/show_bug.cgi?id=176601.

The key idea is to put basic blocks in m_unlinkedBlocks once we have given them a terminal (and know where their successor will be allocated) instead of as soon as possible.
Details are in the Changelog, and in the aforementioned bug.

[Robin Morisset]

Created attachment 322824 [details]
Patch

[Robin Morisset]

This patch is based on the one that I last submitted to https://bugs.webkit.org/show_bug.cgi?id=176601, and takes into account the comments left there by Keith and Saam.

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4084:  WTF_CONCAT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4086:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4091:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6171:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 4 in 6 files


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

[Saam Barati]

Comment on attachment 322824 [details]
Patch

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

r=me with some comments.

> Source/JavaScriptCore/ChangeLog:23
> +        - I made separate allocateBlock routines because the little dance with AdoptRef(* new BasicBlock(...)) was being repeated in lots of places, and typos in that were a major source of bugs during other refactorings

typo: AdoptRef => adoptRef

> Source/JavaScriptCore/ChangeLog:26
> +        - probably some more I forgot..

no need for this. This is assumed in most changelog entries :)

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:82
> +#define VERBOSE_LOG(...) dataLogIf(Options::verboseDFGByteCodeParsing(), __VA_ARGS__)

Why not use the static verbose variable instead of the runtime option? I think until we prove having this runtime switch is actually useful, we should let the compiler eliminate a bunch of the dead code.

Also, FWIW, I'm not sure we'll need all the logging you added. Do you think it's all actually helpful to someone else hacking on this code in the future?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:208
> +    // or they can be untargetable, with bytecodeBegin==UINT_MAX, to be managed manually and not by the linkBlock machinery.

what are the use cases for the not well defined bytecode begin?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1238
> +void ByteCodeParser::makeJumpTo(BasicBlock* block)

Style nit: I think a name more consistent with DFG bytecode parser naming would be: addJumpTo instead of makeJumpTo

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1246
> +void ByteCodeParser::makeJumpTo(unsigned bytecodeIndex)

ditto

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1292
> +    // We first check that we have profiling information about this call, and that it did not behave too polymorphically.

nit: I'd say: "did not behave too polymorphically" => "does not have megamorphic callees" or something similar.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2045
> +

please remove empty line

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4094
> +        shouldContinueParsing
> +#define LAST_OPCODE(name) \

style nit: I'd add a newline between these two macros

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4096
> +        if (m_currentBlock->terminal()) { \

nit: I'd cache the result of terminal() since I'm not 100% sure the compiler will CSE it because it loops and such.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4104
> +                case Jump: \
> +                case Branch: \
> +                case Switch: \
> +                    ASSERT(!m_currentBlock->isLinked); \
> +                    m_inlineStackTop->m_unlinkedBlocks.append(m_currentBlock); \
> +                    break;\
> +                default: break; \

style nit: Webkit style says that we don't indent between switch and case

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4162
>                  addToGraph(Jump, OpInfo(m_currentIndex));

maybe use your helper function here?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6320
> +            // - we may just have returned from an inlined callee that had some early returns and so allocated a continuation block, and the instruction after the call is a jump target.

nit: I'd break this line up across 2 new lines

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6344
> +                // This case only happens if the last instruction was an inlined call with early returns or polymorphic (creating an empty continuation block),

nit: I'd break this line up across 2 new lines

> JSTests/stress/arguments-elimination-varargs-too-many-args-arg-count.js:28
> -            throw "Error: bad result in loop in DFG: " + result.result;
> +            throw "Error: bad result in loop in DFG (result.ftl: " + result.ftl + "): " + result.result + " in iteration " + i;
>      } else {
>          if (result.result != 1)
> -            throw "Error: bad result in loop before DFG: " + result.result;
> +            throw "Error: bad result in loop before DFG (result.ftl: " + result.ftl + "): " + result.result + " in iteration " + i;

Please revert or add a changelog entry for JSTests. I'd vote for reverting since this was probably just used by you for some local debugging.

[Robin Morisset]

Created attachment 323239 [details]
Applied Saam's suggestions + s/PARSER_ASSERT/RELEASE_ASSERT/g

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4084:  WTF_CONCAT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4086:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4091:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6167:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 4 in 5 files


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

[Robin Morisset]

Created attachment 323240 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 323240 [details]
Patch for landing

Clearing flags on attachment: 323240

Committed r223086: <http://trac.webkit.org/changeset/223086>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34898758>