Bug 123056 – DFG dominators: document and rename stuff.

• Component: New Bugs
• Status: RESOLVED FIXED

[Nadav Rotem]

DFG dominators: Fix a bug and document.

[Nadav Rotem]

Created attachment 214637 [details]
Patch

[Filip Pizlo]

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?

[Nadav Rotem]

No. "changed = false" needs to be there.

[Nadav Rotem]

Created attachment 214638 [details]
Patch

[Filip Pizlo]

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 '|=' .

[Nadav Rotem]

Created attachment 214640 [details]
Patch

[Nadav Rotem]

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 ?

[Filip Pizlo]

(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.

[Nadav Rotem]

Created attachment 214659 [details]
Patch

[Nadav Rotem]

In the last patch I fixed the ChangeLog description.

[WebKit Commit Bot]

Comment on attachment 214659 [details]
Patch

Clearing flags on attachment: 214659

Committed r157675: <http://trac.webkit.org/changeset/157675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.