Bug 123056

Summary: DFG dominators: document and rename stuff.
Product: WebKit Reporter: Nadav Rotem <nrotem>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

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.