llvm dev - Nov 2013 - [LLVMdev] possible thumb bug in constant islands

I don't know ARM hardly at all but...

This comment does not seem to match the code.
Or maybe tBfar is a BL?

Also, how does this work if the destination is greater than 2**21?

/// fixupUnconditionalBr - Fix up an unconditional branch whose 
destination is
/// too far away to fit in its displacement field. If the LR register 
has been
/// spilled in the epilogue, then we can use BL to implement a far jump.
/// Otherwise, add an intermediate branch instruction to a branch.
bool
ARMConstantIslands::fixupUnconditionalBr(ImmBranch &Br) {
   MachineInstr *MI = Br.MI;
   MachineBasicBlock *MBB = MI->getParent();
   if (!isThumb1)
     llvm_unreachable("fixupUnconditionalBr is Thumb1 only!");

   // Use BL to implement far jump.
   Br.MaxDisp = (1 << 21) * 2;
   MI->setDesc(TII->get(ARM::tBfar));
   BBInfo[MBB->getNumber()].Size += 2;
   adjustBBOffsetsAfter(MBB);
   HasFarJump = true;
   ++NumUBrFixed;

   DEBUG(dbgs() << "  Changed B to long jump " << *MI);

   return true;
}

Also, how do you know that the LR register has been spilled in the prologue?

Is this always the case with thumb even for a leaf function?

It's of course a reasonable assumption to make that such a large 
function would call something but maybe everything has been inlined.

On 11/18/2013 03:49 PM, reed kotler wrote:
> I don't know ARM hardly at all but...
>
> This comment does not seem to match the code.
> Or maybe tBfar is a BL?
>
> Also, how does this work if the destination is greater than 2**21?
>
> /// fixupUnconditionalBr - Fix up an unconditional branch whose
> destination is
> /// too far away to fit in its displacement field. If the LR register
> has been
> /// spilled in the epilogue, then we can use BL to implement a far jump.
> /// Otherwise, add an intermediate branch instruction to a branch.
> bool
> ARMConstantIslands::fixupUnconditionalBr(ImmBranch &Br) {
>    MachineInstr *MI = Br.MI;
>    MachineBasicBlock *MBB = MI->getParent();
>    if (!isThumb1)
>      llvm_unreachable("fixupUnconditionalBr is Thumb1 only!");
>
>    // Use BL to implement far jump.
>    Br.MaxDisp = (1 << 21) * 2;
>    MI->setDesc(TII->get(ARM::tBfar));
>    BBInfo[MBB->getNumber()].Size += 2;
>    adjustBBOffsetsAfter(MBB);
>    HasFarJump = true;
>    ++NumUBrFixed;
>
>    DEBUG(dbgs() << "  Changed B to long jump " <<
*MI);
>
>    return true;
> }

On Nov 18, 2013, at 3:49 PM, reed kotler <rkotler at mips.com> wrote:
> I don't know ARM hardly at all but...
> 
> This comment does not seem to match the code.
> Or maybe tBfar is a BL?
What does the definition of tBfar say?
> 
> Also, how does this work if the destination is greater than 2**21?
> 
It doesn’t. IIRC, that’s under the category of “if people start having problems
with that limit, we’ll consider complicating the code enough to deal with
it."
> /// fixupUnconditionalBr - Fix up an unconditional branch whose destination
is
> /// too far away to fit in its displacement field. If the LR register has
been
> /// spilled in the epilogue, then we can use BL to implement a far jump.
> /// Otherwise, add an intermediate branch instruction to a branch.
> bool
> ARMConstantIslands::fixupUnconditionalBr(ImmBranch &Br) {
>  MachineInstr *MI = Br.MI;
>  MachineBasicBlock *MBB = MI->getParent();
>  if (!isThumb1)
>    llvm_unreachable("fixupUnconditionalBr is Thumb1 only!");
> 
>  // Use BL to implement far jump.
>  Br.MaxDisp = (1 << 21) * 2;
>  MI->setDesc(TII->get(ARM::tBfar));
>  BBInfo[MBB->getNumber()].Size += 2;
>  adjustBBOffsetsAfter(MBB);
>  HasFarJump = true;
>  ++NumUBrFixed;
> 
>  DEBUG(dbgs() << "  Changed B to long jump " << *MI);
> 
>  return true;
> }
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

On 11/18/2013 06:34 PM, Jim Grosbach wrote:
> On Nov 18, 2013, at 3:49 PM, reed kotler <rkotler at mips.com> wrote:
>
>> I don't know ARM hardly at all but...
>>
>> This comment does not seem to match the code.
>> Or maybe tBfar is a BL?
> What does the definition of tBfar say?
Okay.. but

   // Far jump
   // Just a pseudo for a tBL instruction. Needed to let regalloc know about
   // the clobber of LR.
   let Defs = [LR] in
   def tBfar : tPseudoExpand<(outs), (ins t_bltarget:$target, pred:$p),
                           4, IIC_Br, [], (tBL pred:$p, 
t_bltarget:$target)>,
                           Sched<[WriteBrTbl]>;

Are you expecting LR to be saved because of this Defs?
This is all happening way after register allocation and prologue generation.

This is an unusual situation where LR would not have been saved but it's 
possible to have this
and then you will not return from the function.
>> Also, how does this work if the destination is greater than 2**21?
>>
> It doesn’t. IIRC, that’s under the category of “if people start having
problems with that limit, we’ll consider complicating the code enough to deal
with it."
>
>> /// fixupUnconditionalBr - Fix up an unconditional branch whose
destination is
>> /// too far away to fit in its displacement field. If the LR register
has been
>> /// spilled in the epilogue, then we can use BL to implement a far
jump.
>> /// Otherwise, add an intermediate branch instruction to a branch.
>> bool
>> ARMConstantIslands::fixupUnconditionalBr(ImmBranch &Br) {
>>   MachineInstr *MI = Br.MI;
>>   MachineBasicBlock *MBB = MI->getParent();
>>   if (!isThumb1)
>>     llvm_unreachable("fixupUnconditionalBr is Thumb1 only!");
>>
>>   // Use BL to implement far jump.
>>   Br.MaxDisp = (1 << 21) * 2;
>>   MI->setDesc(TII->get(ARM::tBfar));
>>   BBInfo[MBB->getNumber()].Size += 2;
>>   adjustBBOffsetsAfter(MBB);
>>   HasFarJump = true;
>>   ++NumUBrFixed;
>>
>>   DEBUG(dbgs() << "  Changed B to long jump " <<
*MI);
>>
>>   return true;
>> }
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev