<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 15, 2016 at 3:55 PM, Kyle Butt <span dir="ltr"><<a href="mailto:kyle+llvm@iteratee.net" target="_blank">kyle+llvm@iteratee.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">iteratee added a comment.<br>
<br>
I've looked over the results with and without this patch, and I've come to the conclusion that It's correct to make this less aggressive generally.<br>
<br>
For an unconditional branch, ignoring the threshold because of fallthrough is correct, because there is no runtime cost if the jump happens earlier. It was going to happen anyway.<br></blockquote><div><br></div><div>agree -- the case specialized by your patch should probably be split out as a real no-runtime cost case (but can improve size).</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
For a conditional branch this isn't true. We trade a conditional branch and a jump (which may not execute), for a jump (which always executes) and a conditional branch. Given this, I think it's reasonable to enforce the threshold in this case.<br></blockquote><div><br></div><div>We can easily prove that tail merging will almost always introduce additional runtime cost with conditional branch.</div><div><br></div><div>Assuming MBB1 and MBB2 have two successors Succ1 and Succ2.   After tail merging, assuming Succ1 is the fallthough block from MBB1 before merging, and the fall through block from the merge block after the transformation, then after merging, the branch cost will be</div><div><br></div><div>   Freq(MBB2)  + (Freq(MBB1) * Prob(MBB1->Succ2) + Freq(MBB2) * Prob(MBB2->Succ2))          (1) </div><div><br></div><div>Before tail merging, there are two scenarios:</div><div><br></div><div>Scenario A):   When Succ2 is not a fall through of MBB2, such that there are JCC + JMP instruction in MBB2 . The branch cost in this scenario is </div><div><br></div><div>    Freq(MBB1) * Prob(MBB1->Succ2) + Freq(MBB2)          (2)</div><div><br></div><div><br></div><div>(1) is greater than (2).</div><div><br></div><div><br></div><div>Scenario B): Succ2 is fallthrough of MBB2, then the branch cost before tail merging is</div><div><br></div><div>     Freq(MBB1) * Prob(MBB1->Succ2) + Freq(MBB2) * Prob(MBB2->Succ1)    (3)</div><div><br></div><div><br></div><div>(2) > (3), so   (1) > (3).   In other words, (1) is always worse.</div><div><br></div><div><br></div><div>However as I mentioned in a previous reply to Chandler,  TailMerging is never about runtime performance, but about size reduction.  The pre-layout TailMerging can be dummy (blindly reduce code size), and let later tailDup to undo the previous tail Merging decision depending on the optimization mode (e.g, not undo if Os or Oz is specified).</div><div><br></div><div><br></div><div>Since it is unclear how much size reduction can be gained by allowing conditional branch case , I am fine with your patch, but  you need to add more comments for your patch.   I don't want to see later someone find some size reduction opportunity and remove the check of succ_size() == 1 -- so all of sudden things (about taildup/tailmerge non overlapping assumption are broken.</div><div><br></div><div>It will also be better if you can measure if we lose any size reduction benefit (by building SPEC or some large programs with Os option).<br></div><div><br></div><div>David</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D22317" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22317</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>