[llvm-commits] [llvm] r153903 - in /llvm/trunk/lib/CodeGen: InlineSpiller.cpp LiveRangeEdit.cpp LiveRangeEdit.h RegAllocBasic.cpp RegAllocGreedy.cpp RegAllocPBQP.cpp Spiller.cpp SplitKit.cpp

[Peter Cooper]

On Apr 2, 2012, at 3:48 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:

> 
> On Apr 2, 2012, at 3:22 PM, Pete Cooper <peter_cooper at apple.com> wrote:
> 
>> Author: pete
>> Date: Mon Apr  2 17:22:53 2012
>> New Revision: 153903
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=153903&view=rev
>> Log:
>> Refactored the LiveRangeEdit interface so that MachineFunction, TargetInstrInfo, MachineRegisterInfo, LiveIntervals, and VirtRegMap are all passed into the constructor and stored as members instead of passed in to each method.
> 
> Very nice!
Thanks :)
> 
>> --- llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/LiveRangeEdit.cpp Mon Apr  2 17:22:53 2012
>> @@ -31,13 +31,10 @@
>> 
>> void LiveRangeEdit::Delegate::anchor() { }
>> 
>> -LiveInterval &LiveRangeEdit::createFrom(unsigned OldReg,
>> -                                        LiveIntervals &LIS,
>> -                                        VirtRegMap &VRM) {
>> -  MachineRegisterInfo &MRI = VRM.getRegInfo();
>> +LiveInterval &LiveRangeEdit::createFrom(unsigned OldReg) {
>>  unsigned VReg = MRI.createVirtualRegister(MRI.getRegClass(OldReg));
>> -  VRM.grow();
>> -  VRM.setIsSplitFromReg(VReg, VRM.getOriginal(OldReg));
>> +  VRM->grow();
>> +  VRM->setIsSplitFromReg(VReg, VRM->getOriginal(OldReg));
> 
> If VRM can be NULL, you should assert here, or even better just skip updating it.
I'll assert.  That function shouldn't get called if its NULL.  Or at least it doesn't currently so if someone does in future they'll trigger the assert and can know if its actually ok in their case.
> 
>> @@ -282,12 +264,14 @@
>>    // Shrink just one live interval. Then delete new dead defs.
>>    LiveInterval *LI = ToShrink.back();
>>    ToShrink.pop_back();
>> -    if (foldAsLoad(LI, Dead, MRI, LIS, TII))
>> +    if (foldAsLoad(LI, Dead))
>>      continue;
>>    if (delegate_)
>>      delegate_->LRE_WillShrinkVirtReg(LI->reg);
>>    if (!LIS.shrinkToUses(LI, &Dead))
>>      continue;
>> +    if (!VRM)
>> +      continue;
> 
> This looks suspicious. The code below does more than just updating VRM.
Yeah, it is a little suspicious.  The check before this one avoids running that code if the range was being spilled.  I assumed that if its ok to avoid this code in that case then it should be ok to avoid it here too.  However, I'm happy to be proved wrong and learn more about the register allocator.
> 
>> @@ -94,8 +96,13 @@
>>  ///                empty initially, any existing registers are ignored.
>>  LiveRangeEdit(LiveInterval &parent,
>>                SmallVectorImpl<LiveInterval*> &newRegs,
>> +                MachineFunction &MF,
>> +                LiveIntervals &lis,
>> +                VirtRegMap *vrm,
>>                Delegate *delegate = 0)
> 
> Please update the doxygen comment, and explain what happens when VRM is null.
Will do.

Thanks,
Pete
> 
> /jakob
>