<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">Hi Artyom,</div><div class=""><br class=""></div><div class="">I have a question about this code:</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class=""> // It may be that some blocks are inaccessible going from the CFG exit upwards</div><div class=""> // (e.g. infinite loops); we still need to add them.</div><div class=""> while (Blocks.size() < size) {</div><div class=""> // Find a block that we skipped</div><div class=""> CFG::const_iterator I = cfg->begin(), E = cfg->end();</div><div class=""> for (; I != E; ++I)</div><div class=""> if (!BlockOrder.count(*I))</div><div class=""> break;</div><div class=""> assert(I != E);</div><div class=""> const CFGBlock* block = *I;</div><div class=""> // Add a chain going upwards</div><div class=""> while (!BlockOrder.count(block)) {</div><div class=""> BlockOrder[block] = Blocks.size() + 1;</div><div class=""> Blocks.push_back(block);</div><div class=""> CFGBlock::const_pred_iterator PI = block->pred_begin(),</div><div class=""> PE = block->pred_end();</div><div class=""> for (; PI != PE; ++PI) {</div><div class=""> const CFGBlock* pred = *PI;</div><div class=""> if (pred && !BlockOrder.count(pred)) {</div><div class=""> block = pred;</div><div class=""> break;</div><div class=""> }</div><div class=""> }</div><div class=""> // Chain ends when we couldn't find an unmapped pred</div><div class=""> if (PI == PE) break;</div><div class=""> }</div><div class=""> }</div></blockquote><br class=""></div><div class="">It looks like we will do a complete iteration of the CFG blocks many times, each time we mark a skipped block. Can we invert this loop?</div><div class=""><br class=""></div><div class=""> for (CFG::const_iterator I = cfg->begin(), E = cfg->end(); I != E; ++I) {</div><div class=""> if (Blocks.size() == size)</div><div class=""> break;</div><div class=""><br class=""></div><div class="">That seems far more efficient. We have an invariant that once we visit a block we don't need to visit it again. Inverting the loop makes this an O(n) operation, instead of possibly O(n^2).</div><div class=""><br class=""></div><div class="">Also minor, but I'd prefer this:</div><div class=""><br class=""></div><div class=""> BlockOrder[block] = Blocks.size() + 1;<br class=""> Blocks.push_back(block);</div><div class=""><br class=""></div><div class="">to be written as:</div><div class=""><br class=""></div><div class=""> Blocks.push_back(block);</div><div class=""> BlockOrder[block] = Blocks.size();</div><div class=""><br class=""></div><div class="">That's more consistent (I think) with how we do these things elsewhere.</div><div class=""><br class=""></div><div class="">Note that even this iteration of skipped blocks isn't completely precise; we may hit a block first that dominates other blocks that are all dead.</div><div class=""><br class=""></div><div class="">Perhaps a better approach is to include all unreachable blocks by querying the CFG for unreachable edges. We now try and build the CFG so that all unreachable edges aren't hard pruned, but that you can access the pruned edge via an API call:</div><div class=""><br class=""></div><div class=""><div class=""> /// Get the potentially unreachable block.</div><div class=""> CFGBlock *getPossiblyUnreachableBlock() const {</div><div class=""> return UnreachableBlock.getPointer();</div><div class=""> }</div></div><div class=""><br class=""></div><div class="">That said, we may not have converted the entire CFG to record these "ghost edges". However, if you include the possibly unreachable blocks when you do your initial construction of Blocks, there will be less blocks that are skipped in the first go, and everything will be in the proper order in the worklist.</div><div class=""><br class=""></div><div class="">Also minor, please make all comments be sentences that end with a '.'</div><div class=""><br class=""></div><div class="">For expediency, besides the comments being slightly tweaked, I'm fine with the patch going in as is. That would hopefully unblock Alex's current performance problem. I do think we should invert the above loop though in a subsequent patch.</div><div class=""><br class=""></div><div class="">Ted</div><div class=""><br class=""></div><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 8, 2014, at 8:48 AM, Artyom Skrobov <<a href="mailto:Artyom.Skrobov@arm.com" class="">Artyom.Skrobov@arm.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="WordSection1" style="page: WordSection1; font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">Thank you for the patience – indeed it took me some time to prepare the proper patch, which is now ready for review.<o:p class=""></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">It comes without a regression test, as the only observable change is performance improvement of the analysis.<o:p class=""></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span></div><div style="border-style: solid none none; border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding: 3pt 0cm 0cm;" class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><b class=""><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, sans-serif;" class="">From:</span></b><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, sans-serif;" class=""><span class="Apple-converted-space"> </span>Alexander Kornienko [<a href="mailto:alexfh@google.com" style="color: purple; text-decoration: underline;" class="">mailto:alexfh@google.com</a>]<span class="Apple-converted-space"> </span><br class=""><b class="">Sent:</b><span class="Apple-converted-space"> </span>07 August 2014 23:35<br class=""><b class="">To:</b><span class="Apple-converted-space"> </span>Artyom Skrobov<br class=""><b class="">Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:cfe-commits@cs.uiuc.edu" style="color: purple; text-decoration: underline;" class="">cfe-commits@cs.uiuc.edu</a><span class="Apple-converted-space"> </span>Commits<br class=""><b class="">Subject:</b><span class="Apple-converted-space"> </span>Re: r214064 - Factoring DataflowWorklist out of LiveVariables and UninitializedValues analyses<o:p class=""></o:p></span></div></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><o:p class=""> </o:p></div><div class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class="">Any news here? This keeps biting us.<o:p class=""></o:p></div><div class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><o:p class=""> </o:p></div></div><div class=""><p class="MsoNormal" style="margin: 0cm 0cm 12pt; font-size: 12pt; font-family: 'Times New Roman', serif;">Maybe you can apply this patch first and work on a better solution after that?<o:p class=""></o:p></p><div class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class="">On Tue, Aug 5, 2014 at 6:23 PM, Artyom Skrobov <<a href="mailto:Artyom.Skrobov@arm.com" target="_blank" style="color: purple; text-decoration: underline;" class="">Artyom.Skrobov@arm.com</a>> wrote:<o:p class=""></o:p></div><div class=""><div class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">So, the simplest way to restore the original behaviour seems to be</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">Index: lib/Analysis/LiveVariables.cpp</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">===================================================================</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">--- lib/Analysis/LiveVariables.cpp (revision 214183)</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">+++ lib/Analysis/LiveVariables.cpp (working copy)</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">@@ -451,6 +451,7 @@</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class=""> // Construct the dataflow worklist. Enqueue the exit block as the</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class=""> // start of the analysis.</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class=""> DataflowWorklist worklist(*cfg, AC);</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">+ worklist.quick_hack();</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class=""> llvm::BitVector everAnalyzedBlock(cfg->getNumBlockIDs());</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class=""> // FIXME: we should enqueue using post order.</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">Index: include/clang/Analysis/Analyses/DataflowWorklist.h</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">===================================================================</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">--- include/clang/Analysis/Analyses/DataflowWorklist.h (revision 214183)</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">+++ include/clang/Analysis/Analyses/DataflowWorklist.h (working copy)</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">@@ -53,6 +53,7 @@</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class=""> const CFGBlock *dequeue();</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class=""> void sortWorklist();</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">+ void quick_hack() { enqueuedBlocks.reset(); PO_I = PO_E; }</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class="">};</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: 'Courier New'; color: rgb(31, 73, 125);" class=""> } // end clang namespace</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">This has the effect of disabling the fallback post-order iteration.</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">The reason for the performance regression, however, is related to the iteration order over the CFG: UninitializedValues expects to use the default (i.e. reverse) PostOrderCFGView iteration, whereas LiveVariables expects the opposite order, corresponding to using llvm::GraphTraits<llvm::Inverse<const CFG*> > > as the last parameter in typedef po_iterator.</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">I’m currently thinking of a proper solution which would allow using PostOrderCFGView for iteration in either direction.</span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div><div style="border-style: solid none none; border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding: 3pt 0cm 0cm;" class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><b class=""><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, sans-serif;" class="">From:</span></b><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, sans-serif;" class=""><span class="Apple-converted-space"> </span>Alexander Kornienko [mailto:<a href="mailto:alexfh@google.com" target="_blank" style="color: purple; text-decoration: underline;" class="">alexfh@google.com</a>]<span class="Apple-converted-space"> </span><br class=""><b class="">Sent:</b><span class="Apple-converted-space"> </span>05 August 2014 12:30</span><o:p class=""></o:p></div><div class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><br class=""><b class="">To:</b><span class="Apple-converted-space"> </span>Artyom Skrobov<br class=""><b class="">Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank" style="color: purple; text-decoration: underline;" class="">cfe-commits@cs.uiuc.edu</a><span class="Apple-converted-space"> </span>Commits<o:p class=""></o:p></div></div><div class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><b class="">Subject:</b><span class="Apple-converted-space"> </span>Re: r214064 - Factoring DataflowWorklist out of LiveVariables and UninitializedValues analyses<o:p class=""></o:p></div></div></div><div class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""> <o:p class=""></o:p></div><div class=""><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class="">On another file from the same project the difference in my setup is 2 minutes before the patch vs. 50+ hours (I terminated the analyzer before it finished) after the patch. So it's a rather bad performance regression.<o:p class=""></o:p></div><div class=""><p class="MsoNormal" style="margin: 0cm 0cm 12pt; font-size: 12pt; font-family: 'Times New Roman', serif;"> <o:p class=""></o:p></p></div></div></div></div></div></div></div></div></div><span id="cid:FEE35756-4F3F-4FB6-BF31-1BDF04F590F7@apple.com"><DataflowWorklistInverse.patch></span><span style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">cfe-commits mailing list</span><br style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:cfe-commits@cs.uiuc.edu" style="color: purple; text-decoration: underline; font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">cfe-commits@cs.uiuc.edu</a><br style="font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" style="color: purple; text-decoration: underline; font-family: Helvetica; font-size: 14px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></div></blockquote></div><br class=""></body></html>