[LLVMdev] Possible bug in the ARM backend?

[Evan Cheng]

On Jan 13, 2009, at 2:38 AM, Roman Levenstein wrote:

> Hi again,
>
> 2009/1/13 Evan Cheng <evan.cheng at apple.com>:
>>
>>
>> On Jan 13, 2009, at 12:27 AM, Roman Levenstein <romix.llvm at googlemail.com 
>> >
>> wrote:
>>
>>> 2009/1/13 Evan Cheng <echeng at apple.com>:
>>>>
>>>> On Jan 7, 2009, at 2:48 AM, Roman Levenstein wrote:
>>>>
>>>>> bb368: 0x8fdad00, LLVM BB @0x8fc2c98, ID#1:
>>>>> Predecessors according to CFG: 0x8fdac90 (#0)
>>>>>    %R0<def> = MOVi 0, 14, %reg0, %reg0
>>>>> ***     STR %LR<kill>, %R0<kill>, %reg0, 0, 14, %reg0, Mem:ST(4,4)
>>>>> [0x8fc2d68 + 0]
>>>>>    %LR<def> = LDR <fi#0>, %reg0, 0, 14, %reg0
>>>>>    %SP<def> = ADDri %SP<kill>, 4, 14, %reg0, %reg0
>>>>>    BX_RET 14, %reg0
>>>>
>>>> Ok, ignore my earlier email about BX_RET. The issue is LR should  
>>>> be added
>>>> to
>>>> livein of BB #1.
>>>
>>> Who should do it?
>>> Do you mean that ARM backend/LiveIntervalsAnalysis/LiveVariables
>>> should do it or do you mean that my regalloc should do it?
>>
>> Register allocator should update mbb Livein info.
>
> OK.
>
>>>
>>>
>>>> **** Post Machine Instrs ****
>>>> # Machine code for Insert():
>>>> Live Ins: R0 in VR#1025  R1 in VR#1026
>>>>
>>>> entry: 0x8fdac90, LLVM BB @0x8fc2c48, ID#0:
>>>> Live Ins: %R0 %R1
>>>>    %LR<def> = MOVr %R0, 14, %reg0, %reg0
>>>>    CMPri %R0<kill>, 0, 14, %reg0, %CPSR<imp-def>
>>>>    Bcc mbb<UnifiedReturnBlock,0x8fdad70>, 10, %CPSR<kill>
>>>> Successors according to CFG: 0x8fdad00 (#1) 0x8fdad70 (#2)
>>>>
>>>> bb368: 0x8fdad00, LLVM BB @0x8fc2c98, ID#1:
>>>> Predecessors according to CFG: 0x8fdac90 (#0)
>>>>    %R0<def> = MOVi 0, 14, %reg0, %reg0
>>>>    STR %LR<kill>, %R0<kill>, %reg0, 0, 14, %reg0, Mem:ST(4,4)
>>>> [0x8fc2d68 + 0]
>>>>    BX_RET 14, %reg0
>>>>
>>>> Here the STR is using LR, but there isn't a def earlier.
>>>
>>> May be I overlook something, but doesn't
>>> %LR<def> = MOVr %R0, 14, %reg0, %reg0
>>> in MBB#0 define the LR? It should be enough, or?
>>
>> Every machine basic block must list physical register livein's.
>
> One question, just to be sure I understand you correctly. You mean
> that after the RegAlloc has assigned physical registers to
> LiveIntervals and before it calls the VRM to rewrite the function, it
> should explicitly add lives-ins for each MBB, just like LinearScan
> does it at the end of the RALinScan::linearScan() function???
>
> E.g. like this:
>
>  // Add live-ins to every BB except for entry. Also perform trivial  
> coalescing.
>  MachineFunction::iterator EntryMBB = mf_->begin();
>  SmallVector<MachineBasicBlock*, 8> LiveInMBBs;
>  for (LiveIntervals::iterator i = li_->begin(), e = li_->end(); i !=  
> e; ++i) {
>    LiveInterval &cur = *i->second;
>    unsigned Reg = 0;
>    bool isPhys = TargetRegisterInfo::isPhysicalRegister(cur.reg);
>    if (isPhys)
>      Reg = cur.reg;
>    else if (vrm_->isAssignedReg(cur.reg))
>      Reg = attemptTrivialCoalescing(cur, vrm_->getPhys(cur.reg));
>    if (!Reg)
>      continue;
>    // Ignore splited live intervals.
>    if (!isPhys && vrm_->getPreSplitReg(cur.reg))
>      continue;
>    for (LiveInterval::Ranges::const_iterator I = cur.begin(), E =  
> cur.end();
>         I != E; ++I) {
>      const LiveRange &LR = *I;
>      if (li_->findLiveInMBBs(LR.start, LR.end, LiveInMBBs)) {
>        for (unsigned i = 0, e = LiveInMBBs.size(); i != e; ++i)
>          if (LiveInMBBs[i] != EntryMBB)
>            LiveInMBBs[i]->addLiveIn(Reg);
>        LiveInMBBs.clear();
>      }
>    }
>  }

Only the last part.

>
>
> If it is the case, it is OK. It was not clear for me that one has to
> do it in the regalloc. My assumption was that VRM would do it.

One day I'd like to factor out the code to do it with an analysis  
pass. But unfortunately it's done by the allocators for now. It might  
be ok to move it to VRM. If you want to make that change I am ok with  
it.

Evan
>
>
> Thanks a lot,
>  Roman