[llvm-commits] Priority-Based

[Chris Lattner]

On Nov 15, 2006, at 5:03 PM, Bill Wendling wrote:

> Hi all,
>
> This is meant for a code review and NOT for submission (just yet).  
> This is my implementation of Chow & Hennesey's Priority-Based  
> Coloring Approach to Register Allocation. It's still in the  
> experimental stages (though it compiles the tests). I'd like people  
> to look at it and let me know what you think. The patch included is  
> needed for compilation. You'd place the "RegAllocGraphColoring.cpp"  
> file in the llvm/lib/CodeGen directory. You can use the graph  
> coloring with the commandline:

Cool.

I'll review the patch.  Evan, can you take a first look at the new file?

The patch looks good, but please:

1. Change CreateNewLiveInterval to take LRs by const reference.  This  
will require changing the iterators to be const_iterator instead of  
just iterator's.
2. Please change "int* X" to "int *X" to fit with the rest of the code.
3. Why do this?
+  lv_ = getAnalysisToUpdate<LiveVariables>();
   Isn't lv_ already valid when the method is called?

4. I don't think this is sufficient:

+        MachineOperand& MOp = MI->getOperand(J);
+        if (MOp.isRegister() && MOp.getReg() == LI->reg) {

   In particular, consider if you have two vregs (R1025 and R1027)  
that get coallesced before RA.  When this happens I believe that live  
intervals just marks the two vregs as having the same interval, and  
that interval only has one reg (say R1027).  When this happens, the  
actual machine instrs are not updated.  This means that you can have  
a reference to R1025, even though the interval says it is R1027.

To fix this, try using:

+        MachineOperand& MOp = MI->getOperand(J);
+        if (MOp.isRegister() && rep(MOp.getReg()) == LI->reg) {

which should handle this case.


5. It is unclear to me why you have this loop:
+          for (unsigned K = J + 1; K != MI->getNumOperands(); ++K)
+            if (MI->getOperand(K).isReg() &&
+                MI->getOperand(K).getReg() == LI->reg)
+              MI->getOperand(K).setReg(NewVReg);

If not needed, just remove it.


6. I don't think this is right:

+      // Update live variables if it is available
+      if (lv_)
+        lv_->addVirtualRegisterKilled(NewVReg, MI);

With Evan's recent change, kill/dead markers are now stored on the  
instruction.  If MOp.setReg doesn't modify these markers, you  
probably don't need to do anything to update live vars.

7. The comment:

+/// CreateNewLiveInterval - Create a new live interval with the  
given live
+/// ranges.

Should mention that the created interval has an infinite spill weight.


If you make these changes, go ahead and commit the  
LiveIntervalAnalysis.cpp, LiveIntervalAnalysis.h, and LiveInterval.h  
changes, even before the rest is reviewed.

Thanks Bill!

-Chris