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 _______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits