[llvm-commits] Priority-Based Coloring Approach to Register Allocation

[Bill Wendling]

Hi Evan,

Here are my first thoughts. Sorry this is several days after you sent  
it:

> It's great to have a graph coloring regalloc to compare the current
> implement against. Thanks!
>
No prob. Thanks for the feedback!

> Comments:
>
> 1. INode NeighborList is a std::set<INode*> which is very slow.
> Please use a more efficient data structure. You may have to wrap
> LiveIntervals in something else and give each one a unique id.
>
Okay. I have a BitMatrix data type that I can use. This is what the  
literature suggests using and it will provide the uniqueness that I  
wanted from std::set. Before this goes prime-time, I'll convert it to  
use that.

> 2. Here is something I do have the answer for. But it's wonder
> considering. Graph coloring can be slow. Does it make sense to
> separate nodes by regclasses which cannot conflict. e.g. Do graph
> coloring for all integer nodes and then do another round for all
> floating point ones. This reduces the size of the graph.
>
This is something we could do timings with. It should be easy to  
implement. I honestly don't know the answer. The main amount of time  
for the algorithm comes from the graph building part. But all graph  
builds together would be the same as the unseparated one.

> 3. Is LINodeMap a std::map? Again, you can probably use a more
> efficient data structure if you assign each interval unique id. You
> already have INode, why not use it?
>
This was a bad hack on my part. I needed to know the INode for an  
interval. However, with the BitMatrix, we should have the information  
I need there.

> 4. CalculateInterference(). Why pass LINeighbors.begin() and .end().
> LINodeMap is a RegAlloc ivar.
>
I wanted CalculateInterference() to take the iterators because when  
we go to update the graph, we only want to look at those nodes which  
could be changed if splitting occurs. I'll relook at this to see if  
it can be more efficient.

> 5.
>       /// Test if this live range interferes with the current live
> range and that
>      /// they're both in the same register class.
>      if (CurNode->interferes(NeighborNode) &&
>          RelatedRegClasses.getLeaderValue(RegRC) == RCLeader)
>        CurNode->addNeighbor(NeighborNode);
>      else
>        CurNode->removeNeighbor(NeighborNode);
>
> Why is removeNeighbor() needed? If it is not a neighbor, just don't
> add it to the list?
>
This is for when the graph needs to be updated.

> 6. Making ForbiddenRegs a map seems unnecessary. Why not add a ivar
> to each INode that keeps track which registers are forbbiden?
>
This might be easier with the BitMatrix stuff. But a good suggestion :-)

> 7. SepareUnconstrainedLiveIntervals(): is it possible to create
> separate lists in BuildGraph() rather than separate them later?
>
Sure, that should work.

> 8. CalculatePriority(): The formula in paper is based on spill cost.
> But it doesn't look like you are using this? For future please
> consider LI's which can be rematerialized. Should we give LI's which
> are more restricted (e.g. greater # of ForbiddenRegs, register pairs)
> higher priorities?
>
I mega-cheesed on this priority calculation. There are many things we  
can do to calculate the priority. We can prioritize like you mention  
above.

> 9. PriorityBasedColoring():
>        /// FIXME: This algorithm is pretty gross and will probably
> result in
>        /// massive performance issues!
>
>        if (IsUncolorable(CLI)) {
>          RemoveNodeFromGraph(LINodeMap[CLI]);
>          UncolorableLIs.insert(CLI);
>          DEBUG(std::cerr << "Removing uncolorable LiveInterval: "
>                          << *CLI << "\n");
>        }
>
> Instead of inserting into the UncolorableLIs set and remove them
> outside the loop, perhaps a worklist based approach can be used?
>
Hmm..okay. I think the worklist will work. I probably don't need a  
std::set for this because each uncolorable LI is removed only once.

> 10. GetFreeReg(): That goto inside the nested for loop is evil. :-)

:-)

> How about something like this:
>
> while (I != E) {
>    bool TryAgain = false;
>    for () {
>       if () {
>        TryAgain = true;
>        break;
>       }
>     }
>     if (!TryAgain)
>       break;
>    ++I;
> }
>
> I am not sure how to fix it. But GetFreeReg() seems to be pretty
> expensive? After a register is picked, you update ForbbidenRegs set
> of the neighbors. Is it more desirable to keep track a list of
> candidates? Perhaps we can use a bitmap representation of a register
> class and calculate a unique mask for each physical register before
> coloring?
>
That's possible. Maybe some type of adjacency bitset which will have  
the physical registers which are available for each vreg. I'll have  
to think about the implementation some.

> 11. Please give INode::begin() end() more descriptive names.
>
Okay.

> 12. Loop c in PriorityBasedColoring() seems very expensive. There
> should be a cheaper way to detect whether a LI has to be split. Can
> we cache the result of GetNumMappableRegs()?
>
I tried caching the result of GetNumMappableRegs, but it actually  
changed values on my at one point. This is possibly (probably?) a  
bug, but I didn't delve too deeply into why it was doing it...

> I haven't read SplitLiveInterval in details, so no comment about it  
> yet.
>
Okay. Thanks for the comments so far! I'll work on getting the data  
structures to be more efficient and the other suggestions you made!

-bw