<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Philip,<div class=""><div><br class=""></div><div><blockquote type="cite" class=""><div class="">On 3 Jan 2018, at 13:31, Philip Reames 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="">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" class="">
<div text="#000000" bgcolor="#FFFFFF" class="">
<div class="moz-cite-prefix">Volkan,<br class="">
<br class="">
If you are requiring loop info, you are computing domtree. You
can add a requirement for domtree and pass it in at no additional
runtime cost.<br class=""></div></div></div></blockquote><div><br class=""></div><div>I agree, I'm not sure why it's currently optional in our downstream pass but this would certainly fix the problem.</div><br class=""><blockquote type="cite" class=""><div class=""><div text="#000000" bgcolor="#FFFFFF" class=""><div class="moz-cite-prefix">
Moreover, Anna has made a perfectly reasonable change. We do not
restrict upstream development to prevent breaking downstream
work. We're happy to help guide you in migrating your downstream
code, but you should not expect that downstream breakage (even if
it wasn't easy to fix) would be a cause for an upstream revert.
It is not.<br class=""></div></div></div></blockquote><div><br class=""></div>I don't believe Volkan requested that the patch be reverted so I'm not sure where the harsh tone is coming from. Is there something I've missed?</div><div><br class=""></div><div>As far as I can see, there's a belief that addRequired<LoopInfoWrapperPass>() ensures that the domtree is available but while that's often the case it isn't quite guaranteed as the pass execution log Volkan provided shows. That said, calling UpdateAnalysisInformation() without having a domtree to update is strange at best (the comment says it updates the domtree and you can't update something you don't have) and as Anna explained you need the domtree to update the loop info anyway.</div><div><br class=""></div><div>I currently think that we should something like:</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>assert(DT && "DomTree must be available to update loop info");</div><div>to the code so that passes that update the loop info but didn't ensure the dom tree is available with their own addRequired<DomTreeWrapperPass>() get a better hint as to what they need to do.<br class=""><br class=""><blockquote type="cite" class=""><div class=""><div text="#000000" bgcolor="#FFFFFF" class=""><div class="moz-cite-prefix">
Philip<br class="">
<br class="">
On 01/03/2018 01:15 PM, Volkan Keles via llvm-commits wrote:<br class="">
</div>
<blockquote type="cite" cite="mid:E9A00765-93A2-4E32-B151-DA39171251C0@apple.com" class="">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" 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. 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="">
<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="" moz-do-not-send="true">anna@azul.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8" 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="" moz-do-not-send="true">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="" moz-do-not-send="true">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="" moz-do-not-send="true">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="" moz-do-not-send="true">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="" moz-do-not-send="true">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="" moz-do-not-send="true">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="" moz-do-not-send="true">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>
<br class="">
<fieldset class="mimeAttachmentHeader"></fieldset>
<br class="">
<pre wrap="" class="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote><p class=""><br class="">
</p>
</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="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></div></blockquote></div><br class=""></div></body></html>