[webkit-reviews] review granted: [Bug 126600] DFG fixup phase should be responsible for inserting ValueToInt32's as needed and it should use Phantom to keep the original values alive in case of OSR exit : [Attachment 220550] the patch

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 126600: DFG fixup phase should be responsible for inserting ValueToInt32's
as needed and it should use Phantom to keep the original values alive in case
of OSR exit
https://bugs.webkit.org/show_bug.cgi?id=126600

Attachment 220550: the patch
https://bugs.webkit.org/attachment.cgi?id=220550&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220550&action=review


r+ with comments.

> Source/JavaScriptCore/ChangeLog:10
> +	   was the only exception to that rule, and that was one of the reasons
why we had this bug.

Provide a description of what you did.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1558
> +	   Node* charCode = addToGraph(StringCharCodeAt,
OpInfo(ArrayMode(Array::String).asWord()), get(VirtualRegister(thisOperand)),
get(indexOperand));

Why the VirtualRegister(thisOperand)?  Use thisOperand directly.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1570
> +	   Node* charCode = addToGraph(StringCharAt,
OpInfo(ArrayMode(Array::String).asWord()), get(VirtualRegister(thisOperand)),
get(indexOperand));

Why the VirtualRegister(thisOperand)?  Use thisOperand directly.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1618
> +	       fixIntEdge(m_currentNode->child1()) |
fixIntEdge(m_currentNode->child2());

You really want a binary OR (|) and not a logical OR (||)?