[llvm-commits] Priority-Based

[Bill Wendling]

On Nov 15, 2006, at 6:04 PM, Chris Lattner wrote:
> 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.
>
Will do.

> 2. Please change "int* X" to "int *X" to fit with the rest of the  
> code.
>
ARRRGH!! ;-) But okay. Consistency is a good thing :-)

> 3. Why do this?
> +  lv_ = getAnalysisToUpdate<LiveVariables>();
>   Isn't lv_ already valid when the method is called?
>
I was kind of copying the code from addIntervalsForSpills. It has this  
comment:

   // since this is called after the analysis is done we don't know if
   // LiveVariables is available

I'm not sure if this is also valid for this method...I'll try it  
without this in it.

> 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.
>
Ah! Okay. That's a good point. If this isn't documented, it might be a  
good idea to throw it in a webpage somewhere.

> 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.
>
Hmm...you're right. It looks superfluous. I probably wrote it at 2AM  
or something.

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

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

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

-bw