<div dir="ltr">On 10 October 2013 03:13, Matthias Braun <span dir="ltr"><<a href="mailto:mbraun@apple.com" target="_blank">mbraun@apple.com</a>></span> wrote:<br><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">
You are right that patch was way more complicated that necessary. I rewrote most of it leaving a way simpler patch. Just needed a few invocation changes in the Coalescer patch.<br></blockquote><div></div></div><br></div><div class="gmail_extra">
0009 looks a lot better. I just have two more comments:</div><div class="gmail_extra"><br></div><div class="gmail_extra">First you had:</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">
-  SlotIndex MBBStart, MBBEnd;</div><div class="gmail_extra">-  tie(MBBStart, MBBEnd) = Indexes->getMBBRange(KillMBB);</div><div class="gmail_extra">+  SlotIndex MBBEnd = Indexes->getMBBEndIdx(KillMBB);</div></div><div class="gmail_extra">
<br></div><div class="gmail_extra">Then:</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">       // Check if VNI is live in to MBB.</div><div class="gmail_extra">+      SlotIndex MBBStart, MBBEnd;</div>
<div class="gmail_extra">       tie(MBBStart, MBBEnd) = Indexes->getMBBRange(MBB);</div><div class="gmail_extra"><br></div><div class="gmail_extra">It looks like your second definition is shadowing the first. It seems it was already, though. Or it might just be the way the patch is laid out, and I'm not seeing the whole context.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">Also, here:</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">+void LiveIntervals::pruneValue(LiveInterval &LI, SlotIndex Kill,</div>
<div class="gmail_extra">+                               SmallVectorImpl<SlotIndex> *EndPoints) {</div></div><div class="gmail_extra">+  pruneValue((LiveRange&)LI, Kill, EndPoints);<br></div><div class="gmail_extra">
<br></div><div class="gmail_extra">The naming issue I had still remains. I explain:<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">When looking for a function to prune a current value, I'll find two very similar ones, with one that can be easily cast into another. If I use auto-complete, or don't read the comments, I may pick the wrong one, and it'll be hard for reviewers to spot this problem. If I do read the comments and decide I would never have sub-ranges, I may pick the more restricted one, and if the code changes in the future, and sub-ranges start being valid (or I was wrong), it'll be hard to spot the difference.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">What I'm getting at is that I think the one that iterates the sub ranges should be public and the API we all should call, while the other is the private implementation. Since you're casting the LI into a LR anyway, it'll be just another level of indirection, and since the sub-range iterator will be empty on irrelevant cases, it should be just a comparison and return.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">Does that make sense?</div><div class="gmail_extra"><br></div><div class="gmail_extra">cheers,</div><div class="gmail_extra">--renato</div></div></div>