<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Oct 26, 2010, at 1:03 PM, Evan Cheng wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div 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></blockquote><div><br></div><div>RegAllocBasic is a minimal implementation of RegAllocBase. Maybe it should be called RegAllocMinimal? I could split the .cpp file to make this more clear.</div><div><br></div><div>The only thing a new allocator must do is override selectOrSplit(). You can see from RABasic::selectOrSplit, that not much needs to be implemented to have a working allocator.</div><div><br></div><div>We do need a way to undo an allocation for functional completeness. (RABasic currently passes most but not all benchmarks.) My next step will be to implement LiveIntervalUnion::extract and call it in the uncommon case that we can't find a register for spills.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>2. Does a allocator need to provide its own allocatePhysRegs? Should RegAllocBase provide a base implementation?</div></div></blockquote><div><br></div><div>No. allocatePhysRegs never changes. Any allocator can be plugged in as along it it can solve the global allocation by adhering to two constraints:</div><div><br></div><div>1. It must be incremental vs. iterative (Think linear scan vs. graph coloring.) </div><div><br></div><div>The underlying assumption is that we allocate or split one virtual register at a time until the problem is solved. We do not partially solve the problem, mutate the IR, throw away the solution and retry.</div><div><br></div><div>2. It must be able to express the desired order of allocation as a static priority.</div><div><br></div><div>Currently we assume spill weight can capture the priority. A new allocator can override priority by changing spill weight calculation without otherwise notifying RegAllocBase.</div><div><br></div><div>I imagine linear scan could be reimplemented with this framework. However silly and pointless that would be.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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></blockquote><div><br></div><div>I'm not planning to update LiveStacks in regalloc because Jakob plans to add it to the spiller.</div><div><br></div><div>In general, an allocator implementation will have the choice of updating or recomputing it. Beyond that, I haven't thought much about the tradeoffs.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>4. What's your plan for more efficient LiveSegments implementation?</div></div></blockquote><div><br></div><div>I'll let Jakob answer that. I haven't thought of anything better than a B-btree. </div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>5. What's next? :-)</div></div></blockquote><div><br></div><div>- RABasic handling trivial register reassignment</div><div>- RAGreedy implementation (adding simple reassignment and splitting heuristics)</div><div>- Evaluation of compile-time vs. code quality tradeoffs (what knobs should we give the user?)</div><div><br></div>-Andy</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></div></blockquote></div><br></div></div></blockquote></div><br></body></html>