On Fri, Jul 13, 2012 at 8:03 AM, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:stoklund@2pi.dk" target="_blank">stoklund@2pi.dk</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div style="word-wrap:break-word"><br><div><div><div>On Jul 13, 2012, at 12:20 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:</div><br></div><blockquote type="cite">

<div>On Thu, Jul 12, 2012 at 9:21 PM, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:stoklund@2pi.dk" target="_blank">stoklund@2pi.dk</a>></span> wrote:<br></div><div><div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>When reading your code, it struck me that LiveVariables already did all this. It has a list of kill instructions for each virtual register. If LiveVariables is not available, you are running under -O0, and shouldn't be trying to reschedule instructions anyway.</div>



<br>
You can simply go LV->getVarInfo(Reg).findKill(MBB), and avoid scanning instructions at all.<br></blockquote><div><br></div><div>Wow. I begin to wonder whether large parts of TwoAddress should be re-written to more heavily use LV as it seems like it could simplify a lot more than just this.</div>

</div></div></div></blockquote><div><br></div><div>The plan is to kill off LiveVariables and run LiveIntervals before 2-addr instead. A LiveInterval can find your kill in a single sub-logarithmic lookup. The DistanceMap would be replaced by pre-existing SlotIndexes.</div>

</div></div></blockquote><div><br></div><div>Right, I remember you mentioning this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">

<div><div><blockquote type="cite"><div class="gmail_extra"><div class="gmail_quote">
<div>Patch attached. I've just switched to use LV, but not backed out the various cleanups I made along the way. If you'd rather a minimal patch to switch to hard requiring LV, lemme know and I can do just that.</div>

</div></div></blockquote><div><br></div></div><div>I'm OK with the cleanups, but they should go in a separate commit. The performance issue is handled by simply calling LV->...findKill from findLocalKill(), right?</div>

</div></div></blockquote><div><br></div><div>Correct. I'll probably just skip the cleanups since this code is going to be nuked anyways.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div class="gmail_extra"><div class="gmail_quote">
<div> If the LV list of kill instructions also is large, that is easily fixable in findKill():</div><div>
<br></div><div>I'll run some profiles to see. I don't expect it to be a problem in normal looking code, but maybe other code will trigger other edge cases.</div><div><br></div><div>The other option to just bailing in findKill is to store the Kills in LiveVariable as a DenseMap<MachineBasicBlock*, MachineInstr*>. I don't immediately see anything that would likely make this terrible. The case where there are *tons* of kills, is probably already the case where its worth the extra memory to get some performance.</div>

</div></div></blockquote><div><br></div></div><div>I think it is rare enough that you can simply bail if the kill list is too large.</div></div></div></blockquote><div><br></div><div>Yea, I don't even have a test case where it happens. I can add the code defensively if you're worried.</div>

<div><br></div><div>Final patch attached. I've updated both of the Reschedule functions, and removed findLocalKills completely.</div></div>