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
Summary: DFG fixup phase should be responsible for inserting ValueToInt32's as needed ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 126590
  Show dependency treegraph
 
Reported: 2014-01-07 14:22 PST by Filip Pizlo
Modified: 2014-01-07 16:23 PST (History)
8 users (show)

See Also:


Attachments
the patch (16.83 KB, patch)
2014-01-07 14:30 PST, Filip Pizlo
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2014-01-07 14:22:56 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2014-01-07 14:30:39 PST
Created attachment 220550 [details]
the patch
Comment 2 Michael Saboff 2014-01-07 14:47:09 PST
Comment on attachment 220550 [details]
the patch

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 (||)?
Comment 3 Filip Pizlo 2014-01-07 14:49:26 PST
(In reply to comment #2)
> (From update of attachment 220550 [details])
> 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.

OK!

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

Oops.

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

Oops.

> 
> > 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 (||)?

Yes.  I want to call fixIntEdge() on both edges.  And then if either one of those calls returns true, I want to do the Phantom thing.
Comment 4 Michael Saboff 2014-01-07 14:53:11 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 220550 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=220550&action=review

> > > 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 (||)?
> 
> Yes.  I want to call fixIntEdge() on both edges.  And then if either one of those calls returns true, I want to do the Phantom thing.

Okay.  Then it may make sense to put a comment so someone doesn't come along and ruin things by turning it into a "||".  Something about making sure both calls are made instead of the compiler optimizing out the second call.
Comment 5 Filip Pizlo 2014-01-07 14:55:09 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 220550 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=220550&action=review
> 
> > > > 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 (||)?
> > 
> > Yes.  I want to call fixIntEdge() on both edges.  And then if either one of those calls returns true, I want to do the Phantom thing.
> 
> Okay.  Then it may make sense to put a comment so someone doesn't come along and ruin things by turning it into a "||".  Something about making sure both calls are made instead of the compiler optimizing out the second call.

Added.
Comment 6 Filip Pizlo 2014-01-07 16:23:36 PST
Landed in http://trac.webkit.org/changeset/161465