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

[bugzilla-daemon at webkit.org]

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.