[Webkit-unassigned] [Bug 162125] Enable IC for put_by_id_with_this

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


https://bugs.webkit.org/show_bug.cgi?id=162125

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #308242|review?                     |review-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170426/777f5d66/attachment.html>


More information about the webkit-unassigned mailing list