<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=""><br class=""><div><blockquote type="cite" class=""><div class="">On Apr 8, 2016, at 6:08 PM, Justin Bogner <<a href="mailto:jbogner@apple.com" class="">jbogner@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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="">Mikhail Zolotukhin <</span><a href="mailto:mzolotukhin@apple.com" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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="">mzolotukhin@apple.com</a><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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="">> writes:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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="">Hi,<br class=""><br class="">When I recently was working on PR27157, I discovered a weird behavior<br class="">in our verifier, which made it impossible to discover the bug by using<br class="">'-verify-dom-info - verify' flags. This happens because, when we fully<br class="">unroll a loop, we remove it from the LPM loop queue along with all<br class="">analyses related to it. Here is the code responsible for it:<br class=""><br class=""> {<br class=""> PassManagerPrettyStackEntry X(P, *CurrentLoop->getHeader());<br class=""> TimeRegion PassTimer(getPassTimer(P));<br class=""><br class=""> Changed |= P->runOnLoop(CurrentLoop, *this);<br class=""> }<br class=""> LoopWasDeleted = CurrentLoop->isInvalid();<br class=""><br class=""> if (Changed)<br class=""> dumpPassInfo(P, MODIFICATION_MSG, ON_LOOP_MSG,<br class=""> LoopWasDeleted ? "<deleted>"<br class=""> : CurrentLoop->getHeader()->getName());<br class=""> dumpPreservedSet(P);<br class=""><br class=""> if (LoopWasDeleted) {<br class=""> // Notify passes that the loop is being deleted.<br class=""> deleteSimpleAnalysisLoop(CurrentLoop);<br class=""> } else {<br class=""> // Manually check that this loop is still healthy. This is done<br class=""> // instead of relying on LoopInfo::verifyLoop since LoopInfo<br class=""> // is a function pass and it's really expensive to verify every<br class=""> // loop in the function every time. That level of checking can be<br class=""> // enabled with the -verify-loop-info option.<br class=""> {<br class=""> TimeRegion PassTimer(getPassTimer(&LIWP));<br class=""> CurrentLoop->verifyLoop();<br class=""> }<br class=""><br class=""> // Then call the regular verifyAnalysis functions.<br class=""> verifyPreservedAnalysis(P);<br class=""><br class=""> F.getContext().yield();<br class=""> }<br class=""><br class="">Was it intentional that verifyPreservedAnalysis is under<br class="">!LoopWasDeleted condition? Will anything break if we move<br class="">verifyPreservedAnalysis out of the if?<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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="">Seems like a mistake to me. Try it?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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=""></div></blockquote>It doesn’t break 'ninja check’, but that doesn’t tell much. I can do more testing and then commit if that sounds reasonable. But there is at least one possible argument against it: we’ll verify all analyses after every loop, which is expensive (there is even a comment about it).</div><div><br class=""></div><div>Thinking about it more - maybe the problem isn’t here. If we broke DT, we should be able to detect when LPM is finished, but we don’t. It looks like LPM just doesn't state that DT is preserved. Here is how LPPassManager::getAnalysisUsage looks now:</div><div><div><font face="Menlo" class="">void LPPassManager::getAnalysisUsage(AnalysisUsage &Info) const {</font></div><div><font face="Menlo" class=""> // LPPassManager needs LoopInfo. In the long term LoopInfo class will</font></div><div><font face="Menlo" class=""> // become part of LPPassManager.</font></div><div><font face="Menlo" class=""> Info.addRequired<LoopInfoWrapperPass>();</font></div><div><font face="Menlo" class=""> Info.setPreservesAll();</font></div><div><font face="Menlo" class="">}</font></div><div><br class=""></div><div>Would it make sense to add </div><div><div><font face="Menlo" class=""> Info.addRequired<DominatorTreeWrapperPass>();</font></div><div><font face="Menlo" class=""> Info.addPreserved<DominatorTreeWrapperPass>();</font></div><div>there? I don't know, what '<font face="Menlo" class="">setPreservesAll</font>' is supposed to do, but adding these two lines do have an effect that I'm looking for: pass manager starts running verifyDomTree after the loop pass manager.</div><div><br class=""></div><div>Thanks,</div><div>Michael</div></div></div><div><br class=""></div><div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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="">Thanks,<br class="">Michael</blockquote></div></blockquote></div><br class=""></body></html>