Bug 123056 - DFG dominators: document and rename stuff.
Summary: DFG dominators: document and rename stuff.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-18 22:42 PDT by Nadav Rotem
Modified: 2013-10-19 13:10 PDT (History)
3 users (show)

See Also:

Attachments
Patch (4.59 KB, patch)
2013-10-18 22:44 PDT, Nadav Rotem 		no flags Details | Formatted Diff | Diff
Patch (4.57 KB, patch)
2013-10-18 22:59 PDT, Nadav Rotem 		no flags Details | Formatted Diff | Diff
Patch (4.33 KB, patch)
2013-10-18 23:15 PDT, Nadav Rotem 		no flags Details | Formatted Diff | Diff
Patch (4.33 KB, patch)
2013-10-19 12:37 PDT, Nadav Rotem 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) 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 Nadav Rotem 2013-10-18 22:42:42 PDT 
DFG dominators: Fix a bug and document.
Comment 1 Nadav Rotem 2013-10-18 22:44:52 PDT 
Created attachment 214637 [details]
Patch
Comment 2 Filip Pizlo 2013-10-18 22:49:52 PDT 
Comment on attachment 214637 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGDominators.cpp:90
> -        changed = false;
> +        // Prune dominators in all non entry blocks: backward scan.

It looks like you're no longer setting changed = false in between the forward and backward scan.  Is that right?
Comment 3 Nadav Rotem 2013-10-18 22:55:57 PDT 
No. "changed = false" needs to be there.
Comment 4 Nadav Rotem 2013-10-18 22:59:21 PDT 
Created attachment 214638 [details]
Patch
Comment 5 Filip Pizlo 2013-10-18 23:06:21 PDT 
Comment on attachment 214638 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGDominators.cpp:107
> +    m_scratch.clearAll();
> +
> +    // Find the intersection of dom(preds).
>      m_scratch.set(m_results[block->predecessors[0]->index]);

I'm actually not sure I understand the difference.  FastBitVector::set(const FastBitVector&) is a memcpy().

Hence, the clearAll() call appears to be a no-op.

I agree with the way you restructured the code, though.  It seems that the best thing to do is to drop the clearAll() call and rename set() to something like copyFrom(), to make it clear that it's a copy rather than a bitwise '|=' .
Comment 6 Nadav Rotem 2013-10-18 23:15:13 PDT 
Created attachment 214640 [details]
Patch
Comment 7 Nadav Rotem 2013-10-18 23:17:28 PDT 
oops. I assumed that set is a |=.   Okay I will rename it in another commit.  I want to commit this change and start implementing a better scan: reverse post order.  

Do you think that we should invest time in implementing the Cooper-Harvey-Kennedy algorithm ?
Comment 8 Filip Pizlo 2013-10-19 10:23:49 PDT 
(In reply to comment #7)
> oops. I assumed that set is a |=.   Okay I will rename it in another commit.  I want to commit this change and start implementing a better scan: reverse post order.  
> 
> Do you think that we should invest time in implementing the Cooper-Harvey-Kennedy algorithm ?

That one also gives immediate dominators, right?  I think this would be cool.  I vaguely recall encountering a few situations where having idom's would have been useful.
Comment 9 Nadav Rotem 2013-10-19 12:37:48 PDT 
Created attachment 214659 [details]
Patch
Comment 10 Nadav Rotem 2013-10-19 12:38:37 PDT 
In the last patch I fixed the ChangeLog description.
Comment 11 WebKit Commit Bot 2013-10-19 13:09:57 PDT 
Comment on attachment 214659 [details]
Patch

Clearing flags on attachment: 214659

Committed r157675: <http://trac.webkit.org/changeset/157675>
Comment 12 WebKit Commit Bot 2013-10-19 13:10:00 PDT 
All reviewed patches have been landed.  Closing bug.