<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 12, 2016 at 8:27 AM, Chuang-Yu Cheng <span dir="ltr"><<a href="mailto:cycheng@multicorewareinc.com" target="_blank">cycheng@multicorewareinc.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">cycheng planned changes to this revision.<br>
cycheng added a comment.<br>
<br>
davidxl:<br>
<br>
Yes, -force-precise-rotation-cost=true solve this issue, and the result looks better then this patch. I hope we can enable it by default.<br>
<br>
Test on Power8 with ref data size (largest size), iteration = 1, cpu frequency governor is "performance" (maximum speed), bind on physical-cpu 0:<br>
<br>
- llvm: r269273 (2016 May 12)<br>
- -force-precise-rotation-cost=true v.s. false, > 1.0 means true is good.<br>
- Result:<br>
<br>
400.perlbench 1.00x<br>
401.bzip2 1.00x<br>
403.gcc 1.07x<br>
429.mcf 1.04x<br>
445.gobmk 1.02x<br>
456.hmmer 1.00x<br>
458.sjeng 1.01x<br>
462.libquantum 1.00x<br>
464.h264ref 1.04x<br>
471.omnetpp 1.06x<br>
473.astar 0.97x<br>
483.xalancbmk 0.99x<br>
<br>
<br>
<br>
- llvm r267873 (2016 Apr 28) + this patch<br>
- aggressive-best-top=true v.s. false, > 1.0 means true is good.<br>
- Result:<br>
<br>
400.perlbench 1.01x<br>
401.bzip2 1.01x<br>
403.gcc 1.07x<br>
429.mcf 1.00x<br>
445.gobmk 1.02x<br>
456.hmmer 0.98x<br>
458.sjeng 0.99x<br>
462.libquantum 1.03x<br>
464.h264ref 1.06x<br>
471.omnetpp 1.03x<br>
473.astar 1.00x<br>
483.xalancbmk 0.99x<br>
<br>
So I thought I should use existing rotation strategy.<br></blockquote><div><br></div><div>Thanks for the testing. We will also do more internal testing first.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
By the way, please look at this pattern:<br>
<br>
entry<br>
|<br>
------> loop.header (body)<br>
|97% / \<br>
| /50% \50%<br>
--- latch <--- if.then<br>
|<br>
|3%<br>
loop.end<br>
<br>
Current strategy generates this order: entry loop.header if.then latch loop.end<br>
But I thought this pattern is also eligible to optimize, right?<br></blockquote><div><br></div><div><br></div><div>yes -- it is possible to rotate -- but the current cost model decides it is not a good thing to do. The main reason is that the estimated loop trip count is small (~30), so the relative additional cost due to jumps added to the loop header and the lost fall through to the exit BB from the loop is high. Loop rotation will happen if</div><div><br></div><div>1) -jump-inst-cost=0 option is used, or </div><div>2) increase the loop trip count to larger value such as 100 (with backedge weights of 100 and 1) or larger.</div><div><br></div><div><br></div><div>David</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The cost of loop rotation calculated by current rotation strategy:<br>
<br>
BB#1 ('header') to the top: 273<br>
BB#4 ('if.then') to the top: 297<br>
BB#2 ('latch') to the top: 297<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/CodeGen/AtomicExpandPass.cpp:932-934<br>
@@ -929,3 +931,5 @@<br>
<br>
- Builder.CreateCondBr(Success, ExitBB, LoopBB);<br>
+ // Set LoopBB -> ExitBB and LoopBB -> LoopBB with 50% probability, so block<br>
+ // placement won't apply aggressive-best-top on LoopBB<br>
+ Builder.CreateCondBr(Success, ExitBB, LoopBB, BrWeight);<br>
<br>
----------------<br>
</span><span class="">chandlerc wrote:<br>
> This comment doesn't really help here. What is "aggressive-best-top" and what does it mean?<br>
><br>
> I think what you're actually doing is trying to give probabilities to override the default loop probabilities because the spin loop for an atomic isn't expected to loop often if at all. I'm actually surprised you would have a 50% probability of looping here, I would have expected a relatively low probability of spinning to be more appropriate.<br>
><br>
> I think you should:<br>
><br>
> 1) split this out into an independent change with its own description and test case<br>
> 2) improve this comment to talk generically about the fact that the loop isn't expected to be taken unless there is contention.<br>
><br>
> Then the rest of this change can be predicated on that. I'm assuming that you can test this independently because its actually annotating the IR.<br>
</span>Exactly! And yes, that loop isn't expected to be taken, I should use lower value.<br>
<span class=""><br>
================<br>
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:114-117<br>
@@ -113,1 +113,6 @@<br>
<br>
+static cl::opt<bool><br>
+ AggressiveBestTop("aggressive-best-top",<br>
+ cl::desc("Find best top from all latches even with conditional exit."),<br>
+ cl::init(true), cl::Hidden);<br>
+<br>
----------------<br>
</span><span class="">chandlerc wrote:<br>
> Any particular reason for a flag? Or for calling this aggressive? It seems pretty straight forward to try to select the best loop top among latches.<br>
</span>For testing purpose :P<br>
<span class=""><br>
================<br>
Comment at: test/CodeGen/X86/code_placement_loop_rotation2.ll:8<br>
@@ -7,1 +7,3 @@<br>
; CHECK-LABEL: foo:<br>
+; CHECK: jmp<br>
+; CHECK: callq h<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> By the way, this example shows that the new expected layout is not as optimal as the one when -force-precise-rotation-cost is on (see below CHECK-PROFILE).<br>
><br>
> The total branch cost of the optimal layout [e, f, c, d, h, b, g ] is:<br>
><br>
> C(c->e) + C(f->h) + C(d->f) + C(h->exit) + C(b->c) + C(g->h)<br>
><br>
> while the total cost of aggressive loop top layout [h, b, g, f, c, d, e] is<br>
><br>
> C(c->e) + C(f->h) + C(d->f) + C(h->exit) + C(b->c) + C(g->h) + C(e->f)<br>
><br>
> Edge e->f is in the inner loop and it is a hot edge , the additional cost of C(e->f) can be high.<br>
</span>Thanks for pointing out this : D<br>
<br>
<br>
<a href="http://reviews.llvm.org/D20017" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20017</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>