[webkit-reviews] review denied: [Bug 162125] Enable IC for put_by_id_with_this : [Attachment 308242] Proposed Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Apr 26 13:07:26 PDT 2017


Saam Barati <sbarati at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 162125: Enable IC for put_by_id_with_this
https://bugs.webkit.org/show_bug.cgi?id=162125

Attachment 308242: Proposed Patch

https://bugs.webkit.org/attachment.cgi?id=308242&action=review




--- Comment #26 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 308242
  --> https://bugs.webkit.org/attachment.cgi?id=308242
Proposed Patch

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

I'll keep looking at the rest of the patch, but some basic comments for now.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1926
> +#if USE(JSVALUE64)
> +	   {
> +	       fixEdge<CellUse>(node->child1());
> +	       fixEdge<CellUse>(node->child2());
> +	       break;
> +	   }
> +#endif

Style: This seems slightly too tricky. I'd just have this be its own case, with
a "break" both for 64/32 bit platforms.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:249
> +void SpeculativeJIT::cachedPutByIdWithThis(CodeOrigin codeOrigin, GPRReg
baseGPR, GPRReg valueGPR, GPRReg thisGPR, GPRReg scratchGPR, unsigned
identifierNumber, JITCompiler::JumpList slowPathTargets, SpillRegistersMode
spillMode)

There is only one caller here, which never provides spillmode, please remove
the parameter. Also, since this is a function w/ a single caller, I'd just
remove this function alltogether and just inline it at the only call site.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2944
> +	   if (m_node->child1().useKind() == CellUse &&
m_node->child2().useKind() == CellUse)
> +	       putByIdWithThis(lowJSValue(m_node->child3()),
lowCell(m_node->child1()), lowCell(m_node->child2()));

You unconditionally speculate CellUse in FixupPhase, but then check here if
it's not CellUse. Perhaps FixupPhase's speculation should be conditional?
Otherwise, that "else" branch is most likely dead code.


More information about the webkit-reviews mailing list