possible thumb bug in constant islands

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

possible thumb bug in constant islands

Reed Kotler
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;
}

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: possible thumb bug in constant islands

Reed Kotler
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;
> }


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: possible thumb bug in constant islands

Jim Grosbach
In reply to this post by Reed Kotler

On Nov 18, 2013, at 3:49 PM, reed kotler <[hidden email]> 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
> [hidden email]         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: possible thumb bug in constant islands

Reed Kotler
On 11/18/2013 06:34 PM, Jim Grosbach wrote:
> On Nov 18, 2013, at 3:49 PM, reed kotler <[hidden email]> 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
>> [hidden email]         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: possible thumb bug in constant islands

Jim Grosbach

On Nov 18, 2013, at 6:49 PM, reed kotler <[hidden email]> wrote:

> On 11/18/2013 06:34 PM, Jim Grosbach wrote:
>> On Nov 18, 2013, at 3:49 PM, reed kotler <[hidden email]> 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.
>

Not sure about that aspect. Offhand, it certainly seems like a bug. An extremely unlikely to encounter in practice bug, but still real.

>>> 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
>>> [hidden email]         http://llvm.cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev