Bug 118910 - fourthTier: DFG Nodes should be able to abstractly tell you what they read and what they write
Summary: fourthTier: DFG Nodes should be able to abstractly tell you what they read an...
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: 118935 118940
Blocks: 118749
  Show dependency tree / graph
 
Reported: 2013-07-19 10:57 PDT by Filip Pizlo
Modified: 2013-07-21 21:03 PDT (History)
7 users (show)

See Also:

Attachments
work in progress (5.60 KB, patch)
2013-07-19 10:57 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
work in progress (17.44 KB, patch)
2013-07-19 17:09 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
work in progress (22.17 KB, patch)
2013-07-19 18:52 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
more (31.05 KB, patch)
2013-07-19 21:58 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
clobberizer is done (31.01 KB, patch)
2013-07-19 23:30 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
shockingly, it works (57.50 KB, patch)
2013-07-20 12:27 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
make the changelog even more epic (57.78 KB, patch)
2013-07-20 12:31 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
make the changelog even more lovely (57.73 KB, patch)
2013-07-20 12:35 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
cleanups (58.08 KB, patch)
2013-07-21 15:01 PDT, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (58.76 KB, patch)
2013-07-21 15:19 PDT, Filip Pizlo 		sam: review+
Details | Formatted Diff | Diff
Show Obsolete (9) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2013-07-19 10:57:15 PDT 
Patch forthcoming.
Comment 1 Filip Pizlo 2013-07-19 10:57:52 PDT 
Created attachment 207124 [details]
work in progress
Comment 2 Filip Pizlo 2013-07-19 17:09:03 PDT 
Created attachment 207159 [details]
work in progress
Comment 3 Filip Pizlo 2013-07-19 18:52:56 PDT 
Created attachment 207165 [details]
work in progress
Comment 4 Filip Pizlo 2013-07-19 21:58:19 PDT 
Created attachment 207182 [details]
more
Comment 5 Filip Pizlo 2013-07-19 23:30:03 PDT 
Created attachment 207187 [details]
clobberizer is done
Comment 6 Filip Pizlo 2013-07-20 12:27:03 PDT 
Created attachment 207199 [details]
shockingly, it works

And by "works" I mean that I can dump some clobbers here and there.  Visual inspection confirms that it's as good as it will be before I write a real client that uses this in anger.
Comment 7 Filip Pizlo 2013-07-20 12:31:44 PDT 
Created attachment 207200 [details]
make the changelog even more epic
Comment 8 Filip Pizlo 2013-07-20 12:35:43 PDT 
Created attachment 207201 [details]
make the changelog even more lovely
Comment 9 Filip Pizlo 2013-07-21 15:01:53 PDT 
Created attachment 207213 [details]
cleanups

overlapsReads is renamed to readsOverlap.  Same for writes.

Added a method called didWrites() that just tells you if any writes occurred.
Comment 10 Filip Pizlo 2013-07-21 15:19:00 PDT 
Created attachment 207214 [details]
the patch

added a ClobberSet::addAll() method
Comment 11 Sam Weinig 2013-07-21 18:11:43 PDT 
Comment on attachment 207214 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207214&action=review

Is there anyway to test all this (is it worth it to unit test it)?  Or will I have to wait for LICM?

> Source/JavaScriptCore/dfg/DFGAbstractHeap.h:275
> +    int64_t m_value;

Even though it is not too confusing, you should probably add a short comment explaining the structure of m_value, bitwise.

> Source/JavaScriptCore/dfg/DFGClobberSet.cpp:106
> +    out.print("(Direct:[", sortedListDump(direct()), "], Super:[", sortedListDump(super()), "])");

Please adding timing information so you can have a function called clobberingTime().
Comment 12 Filip Pizlo 2013-07-21 19:22:53 PDT 
(In reply to comment #11)
> (From update of attachment 207214 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207214&action=review
> 
> Is there anyway to test all this (is it worth it to unit test it)?  Or will I have to wait for LICM?

Yeah this will have to wait for LICM and GVN or CSE. 
> 
> > Source/JavaScriptCore/dfg/DFGAbstractHeap.h:275
> > +    int64_t m_value;
> 
> Even though it is not too confusing, you should probably add a short comment explaining the structure of m_value, bitwise.

Will do. 

> 
> > Source/JavaScriptCore/dfg/DFGClobberSet.cpp:106
> > +    out.print("(Direct:[", sortedListDump(direct()), "], Super:[", sortedListDump(super()), "])");
> 
> Please adding timing information so you can have a function called clobberingTime().

I'll consider it.
Comment 13 Filip Pizlo 2013-07-21 21:03:56 PDT 
Landed in http://trac.webkit.org/changeset/152959