[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

[Jakob Stoklund Olesen]

On Oct 22, 2010, at 4:09 PM, Andrew Trick wrote:

> Author: atrick
> Date: Fri Oct 22 18:09:15 2010
> New Revision: 117174
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=117174&view=rev
> Log:
> This is a prototype of an experimental register allocation
> framework. It's purpose is not to improve register allocation per se,
> but to make it easier to develop powerful live range splitting. I call
> it the basic allocator because it is as simple as a global allocator
> can be but provides the building blocks for sophisticated register
> allocation with live range splitting. 
> 
> A minimal implementation is provided that trivially spills whenever it
> runs out of registers. I'm checking in now to get high-level design
> and style feedback. I've only done minimal testing. The next step is
> implementing a "greedy" allocation algorithm that does some register
> reassignment and makes better splitting decisions.

Thanks, Andy! This is very promising.

Some comments below.

> Added: llvm/trunk/lib/CodeGen/RegAllocBase.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegAllocBase.h?rev=117174&view=auto
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/RegAllocBase.h (added)
> +++ llvm/trunk/lib/CodeGen/RegAllocBase.h Fri Oct 22 18:09:15 2010
> @@ -0,0 +1,179 @@
> +#ifndef LLVM_CODEGEN_REGALLOCBASE
> +#define LLVM_CODEGEN_REGALLOCBASE
> +
> +#include "LiveIntervalUnion.h"
> +#include "VirtRegMap.h"
> +#include "llvm/CodeGen/LiveIntervalAnalysis.h"
> +#include "llvm/Target/TargetRegisterInfo.h"
> +#include "llvm/ADT/OwningPtr.h"
> +#include <vector>
> +#include <queue>

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.


> Added: llvm/trunk/lib/CodeGen/LiveIntervalUnion.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/LiveIntervalUnion.h?rev=117174&view=auto
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/LiveIntervalUnion.h (added)
> +++ llvm/trunk/lib/CodeGen/LiveIntervalUnion.h Fri Oct 22 18:09:15 2010
> @@ -0,0 +1,193 @@

> +#include "llvm/CodeGen/LiveInterval.h"

Do you need this header? It looks like SlotIndexes.h is enough.

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

> +/// 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.

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


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

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

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


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

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

> +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().

/jakob