Bug 87205 - DFG should keep captured variables alive until the (inline) return.
: DFG should keep captured variables alive until the (inline) return.
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 85945
: 87813
  Show dependency treegraph
 
Reported: 2012-05-22 21:22 PST by
Modified: 2012-07-31 07:34 PST (History)


Attachments
work in progress (10.06 KB, patch)
2012-05-28 23:37 PST, Filip Pizlo
no flags Review Patch | Details | Formatted Diff | Diff
it's starting to work (12.46 KB, patch)
2012-05-29 00:09 PST, Filip Pizlo
no flags Review Patch | Details | Formatted Diff | Diff
more (16.11 KB, patch)
2012-05-29 14:06 PST, Filip Pizlo
no flags Review Patch | Details | Formatted Diff | Diff
even more (21.44 KB, patch)
2012-05-29 15:06 PST, Filip Pizlo
no flags Review Patch | Details | Formatted Diff | Diff
the patch (33.82 KB, patch)
2012-05-29 16:14 PST, Filip Pizlo
barraclough: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-22 21:22:25 PST
Otherwise OSR exit may decided to set them to Undefined.
------- Comment #1 From 2012-05-22 23:57:50 PST -------
Currently a variable may appear to be dead at a basic block boundary even though it is captured. As a hack, we can peek at the first node of the successor block(s) and check what variables are captured at that node's code origin. But that's just disgusting and wrong.

The right solution is to change how captured variables are flushed. Currently we flush SetLocals by planting a Flush node right after them:

a: SetLocal(thingy)
b: Flush(@a)

But we could instead have a flush right before each SetLocal:

a: Flush(...)
b: SetLocal(thingy)

As well as a Flush for all captured variables at the return site. That will effectively keep variables alive through the whole span of code in which they are captured.
------- Comment #2 From 2012-05-28 23:37:27 PST -------
Created an attachment (id=144458) [details]
work in progress
------- Comment #3 From 2012-05-29 00:09:17 PST -------
Created an attachment (id=144464) [details]
it's starting to work
------- Comment #4 From 2012-05-29 14:06:28 PST -------
Created an attachment (id=144616) [details]
more

Still making sure that I've dotted all of my t's.
------- Comment #5 From 2012-05-29 15:06:25 PST -------
Created an attachment (id=144629) [details]
even more

It's starting to pass tests.
------- Comment #6 From 2012-05-29 16:14:38 PST -------
Created an attachment (id=144638) [details]
the patch
------- Comment #7 From 2012-05-29 16:44:05 PST -------
Landed in http://trac.webkit.org/changeset/118858
------- Comment #8 From 2012-07-31 07:34:00 PST -------
Just for your information, I've got a build warning on my laptop as below. :-)

[  5%] Building CXX object Source/JavaScriptCore/CMakeFiles/javascriptcore_efl.dir/dfg/DFGCapabilities.cpp.o
/home/kangilhan/dev/WebKit/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp: In member function ‘JSC::DFG::ArgumentPosition* JSC::DFG::ByteCodeParser::findArgumentPositionForLocal(int)’:
/home/kangilhan/dev/WebKit/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:361:73: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]