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

[Xerxes Ranby]

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.
>   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.
I have also made sure mov lr, pc gets set the same conditional execution 
predicate as the branch it belongs to.

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?

Xerxes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: condPredMOVLRPC.patch
Type: text/x-patch
Size: 1485 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100722/cc37b7b2/attachment.bin>