[webkit-reviews] review granted: [Bug 177922] Refactor the inliner to simplify block linking : [Attachment 322824] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 9 12:23:07 PDT 2017
Saam Barati <sbarati at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 177922: Refactor the inliner to simplify block linking
https://bugs.webkit.org/show_bug.cgi?id=177922
Attachment 322824: Patch
https://bugs.webkit.org/attachment.cgi?id=322824&action=review
--- Comment #4 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 322824
--> https://bugs.webkit.org/attachment.cgi?id=322824
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.
More information about the webkit-reviews
mailing list