<div dir="ltr">On Mon, Jun 3, 2013 at 6:00 PM, Shuxin Yang <span dir="ltr"><<a href="mailto:shuxin.llvm@gmail.com" target="_blank" class="cremed">shuxin.llvm@gmail.com</a>></span> wrote:<br><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">Author: shuxin_yang<br>
Date: Mon Jun 3 20:00:57 2013<br>
New Revision: 183174<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=183174&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=183174&view=rev</a><br>
Log:<br>
Fix a defect in code-layout pass, improving Benchmarks/Olden/em3d/em3d by about 30%<br>
(4.58s vs 3.2s on an oldish Mac Tower).<br>
<br>
The corresponding src is excerpted bellow. The lopp accounts for about 90% of execution time.<br>
--------------------<br>
cat -n test-suite/MultiSource/Benchmarks/Olden/em3d/make_graph.c<br>
90<br>
91 for (k=0; k<j; k++)<br>
92 if (other_node == cur_node->to_nodes[k]) break;<br>
<br>
The defective layout is sketched bellow, where the two branches need to swap.<br>
------------------------------------------------------------------------<br>
L:<br>
...<br>
if (cond) goto out-of-loop<br>
goto L<br>
<br>
While this code sequence is defective, I don't understand why it incurs 1/3 of<br>
execution time. CPU-event-profiling indicates the poor laoyout dose not increase<br>
in br-misprediction; it dosen't increase stall cycle at all, and it dosen't<br>
prevent the CPU detect the loop (i.e. Loop-Stream-Detector seems to be working fine<br>
as well)...<br></blockquote><div><br></div><div style>Welcome to the fun world of block placement. =] I spent about 4 weeks on the same loop because of this. The final conclusion we came to is that we simply don't have all the data on the hardware, and something else about this loop breaks it. =/ The weird thing is, I fixed this same bug at least once before...</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The root cause of the problem is that the layout pass calls AnalyzeBranch()<br>
with basic-block which is not updated to reflect its current layout.<br></blockquote><div><br></div><div style>Then why isn't the fix to update it? See my comments below. I think you're on the right track here, but I'm pretty convinced that the solution you have here can't be the right solution.</div>
<div style><br></div><div style>Also, you *must* provide test cases for this kind of change. It is far too subtle otherwise, and will just break in the future.</div><div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
rdar://13966341<br>
<br>
Modified:<br>
llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp<br>
<br>
Modified: llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp<br>
URL: <a 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><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 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 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 style><br></div><div style><br></div><div style>B) An alternative solution, which I think I would prefer over (A), is to fix the bug in AnalyzeBranch that causes it to mis-analyze the branch in this case. There doesn't seem to be any reason it can't do the essential work you're describing here or the work that updateTerminators does to compute the correct TBB and FBB information.</div>
<div style><br></div><div style><br></div><div style>C) My favorite solution, which isn't strictly necessary to solve this problem, is to do (B), and then to sink all of this logic for reversing terminators into updateTerminators() and make it do this automatically, factoring it completely out of MachineBlockPlacement.</div>
</div></div></div>