Summary: | DFG dominators: document and rename stuff. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nadav Rotem <nrotem> | ||||||||||
Component: | New Bugs | Assignee: | 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
Nadav Rotem
2013-10-18 22:42:42 PDT
Created attachment 214637 [details]
Patch
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? No. "changed = false" needs to be there. Created attachment 214638 [details]
Patch
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 '|=' . Created attachment 214640 [details]
Patch
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 ? (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. Created attachment 214659 [details]
Patch
In the last patch I fixed the ChangeLog description. Comment on attachment 214659 [details] Patch Clearing flags on attachment: 214659 Committed r157675: <http://trac.webkit.org/changeset/157675> All reviewed patches have been landed. Closing bug. |