[webkit-reviews] review granted: [Bug 68316] DFG JIT does not have full block-local CSE : [Attachment 107779] the patch - updated changelog

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 68316: DFG JIT does not have full block-local CSE
https://bugs.webkit.org/show_bug.cgi?id=68316

Attachment 107779: the patch - updated changelog
https://bugs.webkit.org/attachment.cgi?id=107779&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=107779&action=review


r=me but i'd like some response to my commentary

> Source/JavaScriptCore/dfg/DFGNode.h:176
> +    macro(PutByVal, NodeMustGenerate | NodeClobbersWorld) \
> +    macro(PutByValAlias, NodeMustGenerate | NodeClobbersWorld) \
> +    macro(GetById, NodeResultJS | NodeMustGenerate | NodeClobbersWorld) \
> +    macro(PutById, NodeMustGenerate | NodeClobbersWorld) \
> +    macro(PutByIdDirect, NodeMustGenerate | NodeClobbersWorld) \

Do we really need to consider the gets to clobber the world?  if we know that
the get is a pure object property, then the access is pure

> Source/JavaScriptCore/dfg/DFGPropagator.cpp:732
> +	   case ArithAbs:

Don't we support sqrt(), and min/max?

In the longer term it seems we would probably want a generic handler for pure
intrinsics.

> Source/JavaScriptCore/dfg/DFGPropagator.cpp:-443
> -#if ENABLE(DFG_DEBUG_VERBOSE)
> -    graph.dump(codeBlock);
> -#endif

is this deliberate?