[llvm-commits] [patch] v3 PR7608 ARMv4 JIT forgets to set the lr register when making a indirect function call

[Xerxes Ranby]

Thank you Bob and Jakob for all great tips on how to improve the ARM JIT 
CodeEmitter, this patch have been commited to r109125.
http://llvm.org/viewvc/llvm-project?view=rev&revision=109125

Have a great day
Xerxes

Bob Wilson wrote:
> On Jul 22, 2010, at 1:46 AM, Xerxes Ranby wrote:
>
>   
>> Bob Wilson wrote:
>>     
>>> Wait a minute....  How does EmitMiscBranchInstruction() know how to encode those BX instructions?
>>>       
>> Whats going on in emitMiscBranchInstruction() when encoding BX, BXr9, BMOVPCRX, BMOVPCRXr9 are
>>
>> // Part of binary is determined by TableGn.
>> unsigned Binary = getBinaryCodeForInstr(MI);
>>
>> // Set the conditional execution predicate
>> Binary |= II->getPredicate(&MI) << ARMII::CondShift;
>> // Set branch register containing branch address
>> Binary |= getMachineOpValue(MI, 0);
>>
>> emitWordLE(Binary);
>>
>> All encoding except the conditional execution predicate and the branch register was already set by TableGn.
>> The emitMiscBranchInstruction() only needed to fill in the low nibble with the branch address and the high nibble  with the conditional execution code.
>>     
>
> I knew that, but I didn't see the encoding bits in the .td file.  I'm used to seeing at least some of the encoding bits as parameters to the instruction class, and the patch didn't show enough context for me to see the "let Inst" assignments for those instructions.  Sorry about that!  I'd been wondering how it could possibly have worked....
>
>   
>>>  You need to add the encoding bits in the .td file.  Instead of declaring them as PseudoInsts, declare them as real instructions (similar to BRIND), but set the format to "Pseudo".
>>>  
>>>       
>> I have attached a new patch that sets the format to "Pseudo" for the ABXIx2 class only used by BX, BXr9, BMOVPCRX and BMOVPCRXr9.
>>     
>
> OK.  That's a good way to do it.
>
>   
>> I have also made sure mov lr, pc gets set the same conditional execution predicate as the branch it belongs to.
>>     
>
> Nice catch.  This all looks good to me.  Sorry again for the confusion.
>
>   
>> Index: llvm/lib/Target/ARM/ARMCodeEmitter.cpp
>> ===================================================================
>> --- llvm.orig/lib/Target/ARM/ARMCodeEmitter.cpp    2010-07-19 22:43:33.000000000 +0200
>> +++ llvm/lib/Target/ARM/ARMCodeEmitter.cpp    2010-07-22 10:25:28.000000000 +0200
>> @@ -654,6 +654,19 @@
>>  switch (Opcode) {
>>  default:
>>    llvm_unreachable("ARMCodeEmitter::emitPseudoInstruction");
>> +  case ARM::BX:
>> +  case ARM::BMOVPCRX:
>> +  case ARM::BXr9:
>> +  case ARM::BMOVPCRXr9: {
>> +    // First emit mov lr, pc
>> +    unsigned Binary = 0x01a0e00f;
>> +    Binary |= II->getPredicate(&MI) << ARMII::CondShift;
>> +    emitWordLE(Binary);
>> +
>> +    // and then emit the branch.
>> +    emitMiscBranchInstruction(MI);
>> +    break;
>> +  }
>>  case TargetOpcode::INLINEASM: {
>>    // We allow inline assembler nodes with empty bodies - they can
>>    // implicitly define registers, which is ok for JIT.
>> Index: llvm/lib/Target/ARM/ARMInstrFormats.td
>> ===================================================================
>> --- llvm.orig/lib/Target/ARM/ARMInstrFormats.td    2010-07-22 00:06:34.000000000 +0200
>> +++ llvm/lib/Target/ARM/ARMInstrFormats.td    2010-07-22 09:11:36.000000000 +0200
>> @@ -313,7 +313,7 @@
>> }
>> class ABXIx2<dag oops, dag iops, InstrItinClass itin,
>>             string asm, list<dag> pattern>
>> -  : XI<oops, iops, AddrModeNone, Size8Bytes, IndexModeNone, BrMiscFrm, itin,
>> +  : XI<oops, iops, AddrModeNone, Size8Bytes, IndexModeNone, Pseudo, itin,
>>       asm, "", pattern>;
>> // BR_JT instructions
>>
>> Ok to push?
>>