[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


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