<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Volkan,<br>
<br>
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>
<br>
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>
<br>
Philip<br>
<br>
On 01/03/2018 01:15 PM, Volkan Keles via llvm-commits wrote:<br>
</div>
<blockquote type="cite"
cite="mid:E9A00765-93A2-4E32-B151-DA39171251C0@apple.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
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><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>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
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><br>
</p>
</body>
</html>