[webkit-reviews] review granted: [Bug 135593] Using +s to convert a string to a number badly confuses the DFG : [Attachment 236304] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 135593: Using +s to convert a string to a number badly confuses the DFG
https://bugs.webkit.org/show_bug.cgi?id=135593

Attachment 236304: Patch
https://bugs.webkit.org/attachment.cgi?id=236304&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236304&action=review


R=me.  It would be slightly better if this also included the FTL implementation
of ToNumber.  Also, a test would be cool.  I'm fine with the FTL implementation
being a separate patch.  As a test, maybe you could just land your huffman
thing, if you haven't already?	Even better, land both huffman, and a
microbenchmark for +s.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1283
> +    void fixupToNumber(Node* node)
> +    {
> +	   if (node->child1()->shouldSpeculateInt32()) {
> +	       fixIntOrBooleanEdge(node->child1());
> +	       node->convertToIdentity();
> +	       return;
> +	   }
> +
> +	   if (node->child1()->shouldSpeculateMachineInt()) {
> +	       fixEdge<Int52RepUse>(node->child1());
> +	       node->convertToIdentity();
> +	       node->setResult(NodeResultInt52);
> +	       return;
> +	   }
> +
> +	   if (node->child1()->shouldSpeculateNumber()) {
> +	       fixDoubleOrBooleanEdge(node->child1());
> +	       node->convertToIdentity();
> +	       node->setResult(NodeResultDouble);
> +	       return;
> +	   }
> +    }
> +

You only call this in one place so you might as well inline it. :-)

fixupToPrimitive() had a method because we would call it in more than one
place.	Hopefully that's still the case, or else someone should probably inline
it.