[llvm-commits] [llvm] r117174 - in /llvm/trunk: include/llvm/CodeGen/LinkAllCodegenComponents.h include/llvm/CodeGen/Passes.h lib/CodeGen/CMakeLists.txt lib/CodeGen/LiveIntervalUnion.cpp lib/CodeGen/LiveIntervalUnion.h lib/CodeGen/RegAllocBase.h lib/CodeGen/RegAllocBasic.cpp lib/CodeGen/SplitKit.h

[Andrew Trick]

On Oct 25, 2010, at 2:58 PM, Jakob Stoklund Olesen wrote:
> Please try to limit the header file dependencies.
> 
> - When passing a SmallVector by reference, use SmallVectorImpl, see Spiller.h:
> 
> virtual unsigned selectOrSplit(LiveInterval &lvr,
>                               SmallVectorImpl<LiveInterval*> &splitLVRs) = 0;
> 
> - The rest of the unnecessary headers are pulled in by the premature abstraction of making allocatePhysRegs templated on the comparator.
> 
> Please drop the template, it will clean up the code significantly when you move the implementation into the .cpp file.
> 
> When/if we need the abstraction, we can use virtual functions instead:
> 
>  struct PriQueue {
>    virtual void seed(LiveIntervals&) =0;
>    virtual LiveInterval *get() =0;
>  };
> 
> Since we will be doing significant work on each register, avoiding a virtual call is not worth the template ick.

Agreed, and done.

>> +#include "llvm/CodeGen/LiveInterval.h"
> 
> Do you need this header? It looks like SlotIndexes.h is enough.

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?

>> +struct LiveSegment {
>> +  bool operator<(const LiveSegment &ls) const {
>> +    return start < ls.start || (start == ls.start && end < ls.end);
>> +  }
> 
> I know that struct LiveRange does it too, but why the lexicographical compare? We are only dealing with disjoint segments, right?
> 
> This operator will be quite performance sensitive.

I originally implemented without making the disjoint assumption unless we needed to. But in hindsight I totally agree.

>> +/// Compare a live virtual register segment to a LiveIntervalUnion segment.
>> +inline bool overlap(const LiveRange &lvrSeg, const LiveSegment &liuSeg) {
>> +  return lvrSeg.start < liuSeg.end && liuSeg.start < lvrSeg.end;
>> +}
> 
> 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.

OK

>> +class LiveIntervalUnion {
>> +public:
>> +  // default ctor avoids placement new
>> +  LiveIntervalUnion() : repReg_(0) {}
>> +  
>> +  void init(unsigned repReg) { repReg_ = repReg; }
>> +
>> +  SegmentIter begin() { return segments_.begin(); }
>> +  SegmentIter end() { return segments_.end(); }
>> +
>> +  /// FIXME: !!!!!!!!!!! Keeps a non-const ref
>> +  void unify(LiveInterval &lvr);
>> +
>> +  // FIXME: needed by RegAllocGreedy
>> +  //void extract(const LiveInterval &li);
> 
> Please add comments to public functions.
> 
> I feel your pain on storing non-const references, but just add a comment promising not to change anything.

Oops. Meant to do that.

>> Added: llvm/trunk/lib/CodeGen/LiveIntervalUnion.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/LiveIntervalUnion.cpp?rev=117174&view=auto
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/LiveIntervalUnion.cpp (added)
>> +++ llvm/trunk/lib/CodeGen/LiveIntervalUnion.cpp Fri Oct 22 18:09:15 2010
>> @@ -0,0 +1,167 @@
>> +// Merge a LiveInterval's segments. Guarantee no overlaps.
>> +void LiveIntervalUnion::unify(LiveInterval &lvr) {
>> +  // Add this live virtual register to the union
>> +  LiveVirtRegs::iterator pos = std::upper_bound(lvrs_.begin(), lvrs_.end(),
>> +                                                &lvr, less_ptr<LiveInterval>());
>> +  assert(pos == lvrs_.end() || *pos != &lvr && "duplicate LVR insertion");
> 
> You should be even stricter here, and assert no overlaps.

Yes, meant to do it. Please rereview that code now.

>> +  lvrs_.insert(pos, &lvr);
>> +  // Insert each of the virtual register's live segments into the map
>> +  SegmentIter segPos = segments_.begin();
>> +  for (LiveInterval::iterator lvrI = lvr.begin(), lvrEnd = lvr.end();
>> +       lvrI != lvrEnd; ++lvrI ) {
>> +    LiveSegment segment(lvrI->start, lvrI->end, lvr);
>> +    segPos = segments_.insert(segPos, segment);
>> +    assert(*segPos == segment && "need equal val for equal key");
>> +  }
> 
> You could save some space by coalescing touching segments here, but that can wait until we get a 'real' data structure.

Comment added. Of course, LVR extraction has to handle it too then.

>> +typedef LiveIntervalUnion::SegmentIter SegmentIter;
>> +SegmentIter upperBound(SegmentIter segBegin,
>> +                       SegmentIter segEnd,
>> +                       const LiveRange &lvrSeg)
> 
> Please use anonymous namespaces only for types. Functions should just be declared static.

Yes. Forgot that rule.

>> Added: llvm/trunk/lib/CodeGen/RegAllocBasic.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegAllocBasic.cpp?rev=117174&view=auto
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/RegAllocBasic.cpp (added)
>> +++ llvm/trunk/lib/CodeGen/RegAllocBasic.cpp Fri Oct 22 18:09:15 2010
>> @@ -0,0 +1,259 @@
> 
>> +class RABasic : public MachineFunctionPass, public RegAllocBase
> 
> 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.

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 implementation.

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.

>> +void RABasic::getAnalysisUsage(AnalysisUsage &au) const {
>> +  au.setPreservesCFG();
>> +  au.addRequired<LiveIntervals>();
>> +  au.addPreserved<SlotIndexes>();
>> +  if (StrongPHIElim)
>> +    au.addRequiredID(StrongPHIEliminationID);
>> +  au.addRequiredTransitive<RegisterCoalescer>();
>> +  au.addRequired<CalculateSpillWeights>();
>> +  au.addRequired<LiveStacks>();
>> +  au.addPreserved<LiveStacks>();
>> +  au.addRequired<MachineLoopInfo>();
>> +  au.addPreserved<MachineLoopInfo>();
>> +  au.addRequired<VirtRegMap>();
>> +  au.addPreserved<VirtRegMap>();
>> +  DEBUG(au.addRequired<RenderMachineFunction>());
>> +  MachineFunctionPass::getAnalysisUsage(au);
>> +}
> 
> This would probably be the same for all register allocators using RegAllocBase.

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.

>> +void RABasic::releaseMemory() {
>> +  spiller_.reset(0);
>> +  RegAllocBase::releaseMemory();
>> +}
> 
> Since register allocation is not an analysis used by other passes, you might as well release all memory at the bottom of runOnMachineFunction().

Yes.

-Andy