Bug 118910

Summary:

fourthTier: DFG Nodes should be able to abstractly tell you what they read and what they write

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, ggaren, mark.lam, mhahnenberg, msaboff, oliver, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

118935, 118940

Bug Blocks:

118749

Attachments:

Description	Flags
work in progress	none
work in progress	none
work in progress	none
more	none
clobberizer is done	none
shockingly, it works	none
make the changelog even more epic	none
make the changelog even more lovely	none
cleanups	none
the patch	sam: review+

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