<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
>Also, you *must* provide test cases for this kind of change. It
is far too subtle otherwise, and will just break in the future.<br>
I wish I could, but it is really difficult to fabricate one. <br>
<br>
<blockquote
cite="mid:CAGCO0KhsA38EKAwv=aBo0OxpAnM6yBsjXSj26Q0reTAJ5kNLCg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a class="moz-txt-link-freetext" href="rdar://13966341">rdar://13966341</a><br>
<br>
Modified:<br>
llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp<br>
<br>
Modified: llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp<br>
URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp?rev=183174&r1=183173&r2=183174&view=diff"
target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp?rev=183174&r1=183173&r2=183174&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
(original)<br>
+++ llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp Mon
Jun 3 20:00:57 2013<br>
@@ -991,6 +991,28 @@ void
MachineBlockPlacement::buildCFGChai<br>
Cond.clear();<br>
MachineBasicBlock *TBB = 0, *FBB = 0; // For
AnalyzeBranch.<br>
if (!TII->AnalyzeBranch(*PrevBB, TBB, FBB, Cond))
{<br>
+ // The "PrevBB" is not yet updated to reflect
current code layout, so,<br>
+ // o. it may fall-through to a block without
explict "goto" instruction<br>
+ // before layout, and no longer fall-through
it after layout; or<br>
+ // o. just opposite.<br>
</blockquote>
<div><br>
</div>
<div style="">Yes, and my memory of AnalyzeBranch from when
I wrote this code is that it explicitly had logic (at
least for x86) to handle these cases. We need it to handle
them because updateTerminator (the routine which fixes the
terminator to not have the broken properties you observe
in PrevBB) can only be called when the branch is
analyzable. See the FIXME comment just a few lines up.</div>
</div>
</div>
</div>
</blockquote>
The problem is we cannot blindly call updateTerminator(); it will
complains if the branch is not analyzable. <br>
<br>
<blockquote
cite="mid:CAGCO0KhsA38EKAwv=aBo0OxpAnM6yBsjXSj26Q0reTAJ5kNLCg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ //<br>
+ // AnalyzeBranch() may return erroneous value for
FBB when these two<br>
+ // situations take place. For the first scenario
FBB is mistakenly set<br>
+ // NULL; for the 2nd scenario, the FBB, which is
expected to be NULL,<br>
+ // is mistakenly pointing to "*BI".<br>
</blockquote>
<div><br>
</div>
<div style="">This is either a bug in AnalyzeBranch, or a
bug in us calling AnalyzeBranch. I suspect it is a bug in
one target's implementation of AnalyzeBranch, but the API
and behavior for that routine is so *incredibly*
convoluted and poorly specified that I could believe
either one. However, the fix should not be here! Instead
we should fix updateTerminator and AnalyzeBranch to work
in a more sane way.</div>
</div>
</div>
</div>
</blockquote>
This is the bug in the placement pass -- AnalyzeBranch() is fed with
stale parameter. <br>
That is why the change is at code layout side. <br>
<br>
You may argue AnalyzeBranch() should return non-NULL two branch
target no matter if it has <br>
explicit "goto" instruction or not. I think it is better interface,
however, I don't feel like to <br>
change AnalyzeBranch() this way, as we need to update all its
callers. <br>
<blockquote
cite="mid:CAGCO0KhsA38EKAwv=aBo0OxpAnM6yBsjXSj26Q0reTAJ5kNLCg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div style=""><br>
</div>
<div style="">Let me see if I understand the problem
correctly:</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ //<br>
+ bool needUpdateBr = true;<br>
+ if (!Cond.empty() && (!FBB || FBB == *BI))
{<br>
+ PrevBB->updateTerminator();<br>
</blockquote>
<div><br>
</div>
<div style="">OK, so the goal is that:</div>
<div style=""><br>
</div>
<div style="">1) If the branch is analyzable, but</div>
<div style="">2) We get garbage back from the analysis, then</div>
<div style="">3) We update the terminators, and</div>
<div style="">4) Re-analyze the branch in order to actually
make the terminator reversal decision correctly, and then</div>
<div style="">5) Re-update the terminator?</div>
<div style="">
<br>
</div>
</div>
</div>
</div>
</blockquote>
Yes. It can be simplified into following if we can blindly call
updateTerminator() without triggering assertion. <br>
1) updateTerminator() blindly <br>
2) analyzeBranch().<br>
<br>
<br>
<blockquote
cite="mid:CAGCO0KhsA38EKAwv=aBo0OxpAnM6yBsjXSj26Q0reTAJ5kNLCg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div style="">If I've understood the problem correctly, then
I think there are two possible correct solutions, but both
look somewhat different from this:</div>
<div style=""><br>
</div>
<div style="">A) We could solve in essentially the same way
you have here, but more cleanly. This would be done by
teaching updateTerminator to work even in the cases where
AnalyzeBranch actually fails, so that rather than
asserting, it simply bails out. With this change, we could
write much simpler code:</div>
<div style=""><br>
</div>
<div style="">PrevBB->updateTerminator();</div>
<div style=""><br>
</div>
<div style="">if (TII->AnalyzeBranch(...)) {</div>
<div style=""> if (/* existing code to check for a
profitable reversal case */) {</div>
<div style=""> // reverse the branch and condition</div>
<div style=""> PrevBB->updateTerminator(); // because
we monkeyed with it</div>
<div style=""> }</div>
<div style="">}</div>
<div style=""><br>
</div>
<div style="">Does that make sense? It seems much more clear
than the current code sequence.</div>
</div>
</div>
</div>
</blockquote>
That was exactly my 1st fix, and later on I realize
updateTerminator() cannot be blindly called because <br>
assertion will be triggered if the branch is not analyzable. I
don't want to remove the assertion as it make sense <br>
to me. <br>
<br>
My current fix is slightly different from this one. It just let
updateTerminator() guarded by AnalyzeBranch(), <br>
I add extra few lines of code to make it efficient ---
AnalyzeBranch() and updateTerminator() dosen't seem to be <br>
very expensive; one cycle saved, one cycle earned. <br>
<br>
if (TII->AnalyzeBranch(PrevBB) {<br>
PrevBB->updateTerminator().<br>
TII->AnalyzeBranch(PrevBB); // analyze one more time.<br>
...<br>
reverse etc ...<br>
...<br>
}<br>
<br>
<br>
</body>
</html>