<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Thanks Andy. It looks like a great step. Some questions and comments below:<div><br></div><div>1. How to write a new allocator based on RegAllocBase? Is it as simple as providing a allocation queue priority function and selectOrSplit? How about routines to undo an allocation?</div><div>2. Does a allocator need to provide its own allocatePhysRegs? Should RegAllocBase provide a base implementation?</div><div>3. It would be nice if the new allocators do not have to update LiveStacks but instead compute it after the allocation is done. Do you think it's possible?</div><div>4. What's your plan for more efficient LiveSegments implementation?</div><div>5. What's next? :-)</div><div><br></div><div>Evan</div><div><br></div><div><div><div>On Oct 26, 2010, at 11:37 AM, Andrew Trick wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>On Oct 25, 2010, at 2:58 PM, Jakob Stoklund Olesen wrote:<br><blockquote type="cite">Please try to limit the header file dependencies.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">- When passing a SmallVector by reference, use SmallVectorImpl, see Spiller.h:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">virtual unsigned selectOrSplit(LiveInterval &lvr,<br></blockquote><blockquote type="cite"> SmallVectorImpl<LiveInterval*> &splitLVRs) = 0;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">- The rest of the unnecessary headers are pulled in by the premature abstraction of making allocatePhysRegs templated on the comparator.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Please drop the template, it will clean up the code significantly when you move the implementation into the .cpp file.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">When/if we need the abstraction, we can use virtual functions instead:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> struct PriQueue {<br></blockquote><blockquote type="cite"> virtual void seed(LiveIntervals&) =0;<br></blockquote><blockquote type="cite"> virtual LiveInterval *get() =0;<br></blockquote><blockquote type="cite"> };<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Since we will be doing significant work on each register, avoiding a virtual call is not worth the template ick.<br></blockquote><br>Agreed, and done.<br><br><blockquote type="cite"><blockquote type="cite">+#include "llvm/CodeGen/LiveInterval.h"<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Do you need this header? It looks like SlotIndexes.h is enough.<br></blockquote><br>We could do that by moving certain inline functions, like overlap() into LiveIntervalUnion.cpp. But it only ever makes sense for LiveIntervalUnion users to include LiveInterval, so is it worth scattering the methods?<br><br><blockquote type="cite"><blockquote type="cite">+struct LiveSegment {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ bool operator<(const LiveSegment &ls) const {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ return start < ls.start || (start == ls.start && end < ls.end);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I know that struct LiveRange does it too, but why the lexicographical compare? We are only dealing with disjoint segments, right?<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This operator will be quite performance sensitive.<br></blockquote><br>I originally implemented without making the disjoint assumption unless we needed to. But in hindsight I totally agree.<br><br><blockquote type="cite"><blockquote type="cite">+/// Compare a live virtual register segment to a LiveIntervalUnion segment.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+inline bool overlap(const LiveRange &lvrSeg, const LiveSegment &liuSeg) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ return lvrSeg.start < liuSeg.end && liuSeg.start < lvrSeg.end;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+}<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Please add a note that LiveSegment represents a half-open interval. I am actually planning on switching to closed intervals - it is more natural for callers since the low bits of SlotIndex are symbolic.<br></blockquote><br>OK<br><br><blockquote type="cite"><blockquote type="cite">+class LiveIntervalUnion {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+public:<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // default ctor avoids placement new<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ LiveIntervalUnion() : repReg_(0) {}<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ <br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ void init(unsigned repReg) { repReg_ = repReg; }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ SegmentIter begin() { return segments_.begin(); }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ SegmentIter end() { return segments_.end(); }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ /// FIXME: !!!!!!!!!!! Keeps a non-const ref<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ void unify(LiveInterval &lvr);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // FIXME: needed by RegAllocGreedy<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ //void extract(const LiveInterval &li);<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Please add comments to public functions.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I feel your pain on storing non-const references, but just add a comment promising not to change anything.<br></blockquote><br>Oops. Meant to do that.<br><br><blockquote type="cite"><blockquote type="cite">Added: llvm/trunk/lib/CodeGen/LiveIntervalUnion.cpp<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/LiveIntervalUnion.cpp?rev=117174&view=auto">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/LiveIntervalUnion.cpp?rev=117174&view=auto</a><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">==============================================================================<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">--- llvm/trunk/lib/CodeGen/LiveIntervalUnion.cpp (added)<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+++ llvm/trunk/lib/CodeGen/LiveIntervalUnion.cpp Fri Oct 22 18:09:15 2010<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">@@ -0,0 +1,167 @@<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+// Merge a LiveInterval's segments. Guarantee no overlaps.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+void LiveIntervalUnion::unify(LiveInterval &lvr) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // Add this live virtual register to the union<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ LiveVirtRegs::iterator pos = std::upper_bound(lvrs_.begin(), lvrs_.end(),<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ &lvr, less_ptr<LiveInterval>());<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ assert(pos == lvrs_.end() || *pos != &lvr && "duplicate LVR insertion");<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">You should be even stricter here, and assert no overlaps.<br></blockquote><br>Yes, meant to do it. Please rereview that code now.<br><br><blockquote type="cite"><blockquote type="cite">+ lvrs_.insert(pos, &lvr);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ // Insert each of the virtual register's live segments into the map<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ SegmentIter segPos = segments_.begin();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ for (LiveInterval::iterator lvrI = lvr.begin(), lvrEnd = lvr.end();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ lvrI != lvrEnd; ++lvrI ) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ LiveSegment segment(lvrI->start, lvrI->end, lvr);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ segPos = segments_.insert(segPos, segment);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ assert(*segPos == segment && "need equal val for equal key");<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">You could save some space by coalescing touching segments here, but that can wait until we get a 'real' data structure.<br></blockquote><br>Comment added. Of course, LVR extraction has to handle it too then.<br><br><blockquote type="cite"><blockquote type="cite">+typedef LiveIntervalUnion::SegmentIter SegmentIter;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+SegmentIter upperBound(SegmentIter segBegin,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ SegmentIter segEnd,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ const LiveRange &lvrSeg)<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Please use anonymous namespaces only for types. Functions should just be declared static.<br></blockquote><br>Yes. Forgot that rule.<br><br><blockquote type="cite"><blockquote type="cite">Added: llvm/trunk/lib/CodeGen/RegAllocBasic.cpp<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegAllocBasic.cpp?rev=117174&view=auto">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegAllocBasic.cpp?rev=117174&view=auto</a><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">==============================================================================<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">--- llvm/trunk/lib/CodeGen/RegAllocBasic.cpp (added)<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+++ llvm/trunk/lib/CodeGen/RegAllocBasic.cpp Fri Oct 22 18:09:15 2010<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">@@ -0,0 +1,259 @@<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">+class RABasic : public MachineFunctionPass, public RegAllocBase<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Instead of using multiple inheritance, would it make sense for RegAllocBase to be a MachineFunctionPass? It seems that any register allocator using RegAllocBase would need the same pass initialization code and so on.<br></blockquote><br>I was also tempted to make RegAlloc an abstract MachineFunctionPass. In fact I originally did that, and it was a bit easier write the code. But I want to give you some time to reconsider, because I don't think that design follows what you originally asked for: an implementation of the basic allocation pass that would never change and serve as a clean reference point and boilerplate code for extensions. It will be easier for others to understand the design and how to extend it if we keep RegAllocBase pure so that it only handles logic fundamental to the register allocation data structures and algorithms. Each regalloc subclass is a different type of pass, so it is responsible for all pass-related implementation. If RegAllocBase is derived from MachineFunctionPass then it will implement some of the pass setup and dependencies and the subclasses will each have to figure out what's left over and add any new dependencies--I think that's bad design and leads to scattered implement!<br> ation.<br><br>I would not violate good design principles just to avoid multiple inheritance. I think that's a separate issue. Multiple inheritance was just an easy shortcut to avoid defining another class (e.g. RegAllocBasicImpl + RegAllocBasicPass), but I would be happy to do that if you think it's worthwhile.<br><br><blockquote type="cite"><blockquote type="cite">+void RABasic::getAnalysisUsage(AnalysisUsage &au) const {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.setPreservesCFG();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addRequired<LiveIntervals>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addPreserved<SlotIndexes>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (StrongPHIElim)<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addRequiredID(StrongPHIEliminationID);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addRequiredTransitive<RegisterCoalescer>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addRequired<CalculateSpillWeights>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addRequired<LiveStacks>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addPreserved<LiveStacks>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addRequired<MachineLoopInfo>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addPreserved<MachineLoopInfo>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addRequired<VirtRegMap>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ au.addPreserved<VirtRegMap>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ DEBUG(au.addRequired<RenderMachineFunction>());<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ MachineFunctionPass::getAnalysisUsage(au);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+}<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This would probably be the same for all register allocators using RegAllocBase.<br></blockquote><br>I think the highest priority is to list all dependencies for a pass in one place for readability. I'm assuming that the common dependencies will hardly change, and if they do it's trivial to correct all passes, but that each pass (subclass) will add dependencies. This may be a rare case in which it's actually desirable to copy the common code, rather than scatter the implementation. If it's really important to factor pass setup, then the same reasoning should apply to all register allocators.<br><br><blockquote type="cite"><blockquote type="cite">+void RABasic::releaseMemory() {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ spiller_.reset(0);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ RegAllocBase::releaseMemory();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+}<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Since register allocation is not an analysis used by other passes, you might as well release all memory at the bottom of runOnMachineFunction().<br></blockquote><br>Yes.<br><br>-Andy<br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></div></blockquote></div><br></div></body></html>