<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=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Jan 5, 2018, at 11:10 AM, Philip Reames <<a href="mailto:listmail@philipreames.com" class="">listmail@philipreames.com</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">On 01/03/2018 02:20 PM, Daniel Sanders
wrote:<br class="">
</div>
<blockquote type="cite" cite="mid:130DD9DF-432D-4477-AF43-C009D92B6758@apple.com" class="">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" class="">
Hi Philip,
<div class="">
<div class=""><br class="">
</div>
<div class="">
<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="" moz-do-not-send="true">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 class=""><br class="">
</div>
<div class="">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 class=""><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>
</blockquote>
Let me apologize for the tone. I hadn't intended to come across as
harsh here, but re-reading I see that I definitely did. Sorry.<br class=""></div></div></blockquote><div><br class=""></div><div>No problem, I probably should have explained what I wanted to say clearly. I didn’t mean to request to change the implementation, I thought it would be useful to check if DT is provided as it may not be available always.</div><div><br class=""></div><div>Volkan</div><br class=""><blockquote type="cite" class=""><div class=""><div text="#000000" bgcolor="#FFFFFF" class="">
<br class="">
My concern was that we had a request to change an implementation
strategy due to effects on downstream code. We generally don't want
to be taking on the maintenance burden for keeping out of tree code
working. We do in selective cases where the burden is small and
clearly motivated, but as of the time of my response, I was not
convinced this example met that bar.<br class="">
<br class="">
I believe the rest of this is discussed downthread, so I'll leave it
there.<br class="">
<br class="">
<blockquote type="cite" cite="mid:130DD9DF-432D-4477-AF43-C009D92B6758@apple.com" class="">
<div class="">
<div class=""><br class="">
</div>
<div class="">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 class=""><br class="">
</div>
<div class="">I currently think that we should something like:</div>
<div class=""><span class="Apple-tab-span" style="white-space:pre"> </span>assert(DT
&& "DomTree must be available to update loop info");</div>
<div class="">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 class="" wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org" moz-do-not-send="true">llvm-commits@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" moz-do-not-send="true">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="" moz-do-not-send="true">llvm-commits@lists.llvm.org</a><br class="">
<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><br class="">
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote><p class=""><br class="">
</p>
</div>
</div></blockquote></div><br class=""></body></html>