<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="">
Thanks for the clarification. The idiom I’ve seen in most passes is adding both DT and LI as required, which keeps both of them live during the pass.
<div class=""><br class="">
</div>
<div class="">Also, the assert about requiring DT while updating LI makes sense. I’ll add it to the function.</div>
<div class=""><br class="">
</div>
<div class=""><br class="">
</div>
<div class="">Anna<br class="">
<div>
<blockquote type="cite" class="">
<div class="">On Jan 3, 2018, at 5:30 PM, Daniel Sanders <<a href="mailto:daniel_l_sanders@apple.com" class="">daniel_l_sanders@apple.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">
<br class="">
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">On 3 Jan 2018, at 13:38, Anna Thomas via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">
<div class=""><br class="">
</div>
<div class="">
<div class="">
<blockquote type="cite" class="">
<div class="">On Jan 3, 2018, at 4:15 PM, Volkan Keles <<a href="mailto:vkeles@apple.com" class="">vkeles@apple.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">
Hi Anna,
<div class=""><br class="">
</div>
<div class="">As you can see in my previous e-mail, it may be freed if it’s not required later. It’s required to compute LoopInfo, but it’s not required to use LoopInfo.</div>
</div>
</div>
</blockquote>
<div class="">There’s really no difference between computing and using LoopInfo. Whenever we compute or use LI in a pass, it means the LI analysis is *live*, which means the DT should be live. </div>
</div>
</div>
</div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">I don't think this is quite correct. addRequired<LoopInfoWrapperPass>() ensures that the analysis is run and allows you to query the results, but I don't think it ensures that you still have the other analyses needed update/maintain it. In order
to do that you need to use addRequiredTransitive<LoopInfoWrapperPass>() which should effectively require the domtree too.</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">
<div class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">
<div class="">In UpdateAnalysisInformation, DT is not only updated but also used, so it’s no longer optional. I think we should make it a reference or at least check if it’s provided or not. Does that make sense?<br class="">
</div>
</div>
</div>
</blockquote>
<div class="">
<div class="">Yes, as I’ve stated in the previous email, you can check for DT and avoid updating LI, and this will unbreak your bot, </div>
<div class="">but this is really really *bad* - you will no longer get crashes, but your LI will be corrupted, even though LI was passed in for update!</div>
<div class="">Note that we cannot make DT a reference or a required parameter because the callers of UpdateAnalysisInformation need not pass in DT or LI if the analysis need not be updated.</div>
</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">
<div class=""><br class="">
Volkan<br class="">
<div class="">
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">On Jan 3, 2018, at 12:35 PM, Anna Thomas <<a href="mailto:anna@azul.com" class="">anna@azul.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">
<div class="">Hi Volkan,</div>
<div class="">
<div class=""><br class="">
</div>
<div class=""><br class="">
<div class="">
<blockquote type="cite" class="">
<div class="">On Jan 3, 2018, at 2:31 PM, Volkan Keles <<a href="mailto:vkeles@apple.com" class="">vkeles@apple.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">
Hi Anna,
<div class=""><br class="">
</div>
<div class="">Yes, LI is computed using DT, but this doesn’t mean DT is required.
</div>
</div>
</div>
</blockquote>
<div class="">If LI is available for a pass at a particular point, it means DT is available at that point as well (or can be retrieved).
<div class="">This is because the required pass for LI is the DomTreeWrapperPass. The DT pass will only be free’d when the LoopInfo pass is free’d.</div>
<div class="">For example, lets say we wanted to recompute LI using the analyze function - we need to pass in a dom tree.</div>
<div class=""><br class="">
</div>
</div>
Could you pls verify if this is true in your case and if so, it will provide a quick and correct fix for your pass.</div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">
<div class="">Here is what llc with the option `-debug-pass=Executions` produces:<br class="">
<br class="">
[2018-01-03 11:24:29.528119000] 0x7f90546035a0 Executing Pass 'Dominator Tree Construction' on Function ‘foo’…<br class="">
…<br class="">
[2018-01-03 11:24:29.528212000] 0x7f90546035a0 Executing Pass 'Natural Loop Information' on Function ‘foo’…<br class="">
…<br class="">
[2018-01-03 11:24:29.528834000] 0x7f90546035a0 Freeing Pass 'Dominator Tree Construction' on Function ‘foo’…<br class="">
<br class="">
So, DominatorTree may not be available at this point. I think DT shouldn’t be optional in this function if it’s required.</div>
</div>
</div>
</blockquote>
<div class="">
<div class="">Neither DT nor LI is required by this function - if neither is provided, we don’t do anything in UpdateAnalysisInformation.</div>
<div class="">
<div class="">We should be more strict here and check that if LI is being asked to be updated, we need DT as well. This will unbreak your bot </div>
<div class="">but will no longer preserve LI unless DT is passed. </div>
</div>
<div class=""><br class="">
</div>
<div class="">This seems like a valid thing to do given the reasoning above (DT is available as long as LI is available). </div>
<div class=""><br class="">
</div>
<div class=""><br class="">
</div>
<div class="">Anna</div>
</div>
<blockquote type="cite" class="">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">
<div class=""><br class="">
Volkan<br class="">
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">On Jan 3, 2018, at 6:09 AM, Anna Thomas <<a href="mailto:anna@azul.com" class="">anna@azul.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<blockquote type="cite" class="" 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-size-adjust: auto; -webkit-text-stroke-width: 0px;">
<div class=""><br class="Apple-interchange-newline">
On Jan 2, 2018, at 10:06 PM, Friedman, Eli <<a href="mailto:efriedma@codeaurora.org" class="">efriedma@codeaurora.org</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class=""><span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">On
1/2/2018 3:45 PM, Volkan Keles via llvm-commits wrote:</span><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">
<blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">
+llvm-commits<br class="">
<br class="">
<blockquote type="cite" class="">On Jan 2, 2018, at 8:25 AM, Anna Thomas via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:<br class="">
<br class="">
Author: annat<br class="">
Date: Tue Jan 2 08:25:50 2018<br class="">
New Revision: 321653<br class="">
<br class="">
URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project?rev=321653&view=rev" class="">http://llvm.org/viewvc/llvm-project?rev=321653&view=rev</a><br class="">
Log:<br class="">
[BasicBlockUtils] Check for unreachable preds before updating LI in UpdateAnalysisInformation<br class="">
<br class="">
Summary:<br class="">
We are incorrectly updating the LI when loop-simplify generates<br class="">
dedicated exit blocks for a loop. The issue is that there's an implicit<br class="">
assumption that the Preds passed into UpdateAnalysisInformation are<br class="">
reachable. However, this is not true and breaks LI by incorrectly<br class="">
updating the header of a loop.<br class="">
<br class="">
One such case is when we generate dedicated exits when the exit block is<br class="">
a landing pad (through SplitLandingPadPredecessors). There maybe other<br class="">
cases as well, since we do not guarantee that Preds passed in are<br class="">
reachable basic blocks.<br class="">
<br class="">
The added test case shows how loop-simplify breaks LI for the outer loop (and DT in turn)<br class="">
after we try to generate the LoopSimplifyForm.<br class="">
<br class="">
Reviewers: davide, chandlerc, sanjoy<br class="">
<br class="">
Reviewed By: davide<br class="">
<br class="">
Subscribers: llvm-commits<br class="">
<br class="">
Differential Revision:<span class="Apple-converted-space"> </span><a href="https://reviews.llvm.org/D41519" class="">https://reviews.llvm.org/D41519</a><br class="">
<br class="">
Modified:<br class="">
llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp<br class="">
llvm/trunk/test/Transforms/LoopSimplify/unreachable-loop-pred.ll<br class="">
<br class="">
Modified: llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp<br class="">
URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp?rev=321653&r1=321652&r2=321653&view=diff" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp?rev=321653&r1=321652&r2=321653&view=diff</a><br class="">
==============================================================================<br class="">
--- llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp (original)<br class="">
+++ llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp Tue Jan 2 08:25:50 2018<br class="">
@@ -323,6 +323,12 @@ static void UpdateAnalysisInformation(Ba<br class="">
bool IsLoopEntry = !!L;<br class="">
bool SplitMakesNewLoopHeader = false;<br class="">
for (BasicBlock *Pred : Preds) {<br class="">
+ // Preds that are not reachable from entry should not be used to identify if<br class="">
+ // OldBB is a loop entry or if SplitMakesNewLoopHeader. Unreachable blocks<br class="">
+ // are not within any loops, so we incorrectly mark SplitMakesNewLoopHeader<br class="">
+ // as true and make the NewBB the header of some loop. This breaks LI.<br class="">
+ if (!DT->isReachableFromEntry(Pred))<br class="">
</blockquote>
Hi Anna,<br class="">
<br class="">
This change breaks our internal bots because DT might be nullptr as it is optional. Is there another way to check this?<br class="">
</blockquote>
<br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">
<span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">This
code only executes if the LoopInfo pointer is non-null, and LoopInfo requires a DominatorTree to compute, so you probably have a domtree somewhere even if your pass doesn't explicitly require it.</span><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">
</div>
</blockquote>
<div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
Agree with Eli here. If LI is being passed in to this method, it makes sense to pass in the DT as well (which exists because LI was computed using DT). </div>
<div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
Reachability analysis without DT would be from first principles because LI does not contain that information. </div>
<div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<br class="">
</div>
<div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
<br class="">
</div>
<div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">
Anna</div>
<blockquote type="cite" class="" 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-size-adjust: auto; -webkit-text-stroke-width: 0px;">
<div class=""><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">
<span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">-Eli</span><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">
<br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">
<span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">--<span class="Apple-converted-space"> </span></span><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">
<span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">Employee
of Qualcomm Innovation Center, Inc.</span><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">
<span class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">Qualcomm
Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project</span></div>
</blockquote>
</div>
</blockquote>
</div>
<br class="">
</div>
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</div>
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</div>
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</div>
_______________________________________________<br class="">
llvm-commits mailing list<br class="">
<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="">
</div>
</blockquote>
</div>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</body>
</html>