[llvm-commits] [PATCH] thumb t2b fix

Greg Fitzgerald garious at gmail.com
Thu Sep 27 10:00:24 PDT 2012


Ping.

same patch attached

On Wed, Sep 26, 2012 at 10:27 AM, Greg Fitzgerald <garious at gmail.com> wrote:
> The attached patch is the same as before but includes a missing
> function declaration.
>
> Thanks,
> Greg
>
>
> On Mon, Sep 24, 2012 at 5:15 PM, Greg Fitzgerald <garious at gmail.com> wrote:
>>> I believe that "target" only really exists in the .td file and cannot
>>> be anything but S:J1:J2:imm10:imm11 by definition (it's assigned
>>> directly to Inst{}!).
>>
>> 'target' maps to the input value to the decoder method and the return
>> value of the encoder method.
>>
>>
>>> I think Owen would prefer that getImm() returns S:J1:J2:imm10:imm11 too.
>>> He specifically refers to the MCOperand as having that value.
>>
>> Since no instructions in the ARM backend are currently written this
>> way, when making bug fixes, can we aim to be consistent with the
>> existing code?
>>
>> The more I think about it, I'm really not on board with the long-term
>> plan of exposing the encoding details to the caller of getImm().  I
>> think that method should return 'imm32', not 'S:J1:J2:imm10:imm11'.
>> Returning imm32 keeps relaxation and instruction selection simple.
>> But in any case, can we please move this aspect of the discussion to
>> another thread?  If we're going to go this route, we should make the
>> change to all ARM instructions at once, and as a separate patch.
>>
>> Attached is an updated patch ready for review.  It corrects an
>> encoding bug in the J1/J2 bits, adds more unit tests, and simplifies
>> the code by making a symmetric encoder/decoder for 'target'.
>>
>> Thanks,
>> Greg
>>
>>
>>
>> On Thu, Sep 20, 2012 at 1:20 PM, Tim Northover <t.p.northover at gmail.com> wrote:
>>> Hi Greg,
>>>
>>>> Thanks for the review Tim.  Just to verify, when you say "MCInst's
>>>> immediate", we should distinguish the value returned by "getImm()"
>>>> from the value in 'target' in the .td file.  I believe getImm() should
>>>> return the sign-extended 'imm32' and that 'target' can contain
>>>> 'S:J1:J2:imm10:imm11'.
>>>
>>> I believe that "target" only really exists in the .td file and cannot
>>> be anything but S:J1:J2:imm10:imm11 by definition (it's assigned
>>> directly to Inst{}!). (Intepreting the words of the prophet ;-)) I
>>> think Owen would prefer that getImm() returns S:J1:J2:imm10:imm11 too.
>>> He specifically refers to the MCOperand as having that value.
>>>
>>>> Otherwise, you're asking the user to do
>>>> redundant bit-twiddling during instruction selection and relaxation,
>>>
>>> At a casual glance, selection is likely fine (it's just going to give
>>> a label with an attached fixup: getImm() == 0 is acceptable).
>>> Relaxation has to account for all the complexity regardless of which
>>> route is taken.
>>>
>>> The other relevant bits are:
>>> + Disassembler: complex now; no special handling in the proposed solution.
>>> + InstPrinter: simple now; more complex in proposed solution.
>>> + Encoder: complex now; simple in proposed solution.
>>> + AsmParser: simple now; complex in proposed solution.
>>>
>>> We're shuffling the complexity around to achieve two goals:
>>> 1. Fits in with other instructions in having operands to an MCInst
>>> already encoded where possible.
>>> 2. Make and MCInst is intrinsically valid (not entirely convinced on
>>> this one, but it's certainly a step closer to that goal than the
>>> status quo).
>>>
>>>> My interpretation of Owen's suggestion is to have a symmetric
>>>> encoder/decoder on 'target' and remove the decoder on the 't2B'
>>>> definition.
>>>
>>> I'm really not sure what you mean by "target"here. The only reference
>>> I see is an arbitrary field name in the .td file. During runtime the
>>> primary place this "target" exists is as an MCOperand. Various
>>> components have to either create or interpret that value.
>>>
>>>> My preference, generally speaking, is to go "minimally invasive" when fixing a
>>>> bug, and then refactor as a separate "cleanup" patch.[...] Sound good?
>>>
>>> I obviously have no problem with the first part of that, but I think
>>> you're misinterpreting Owen's goals for the long term.
>>>
>>> Tim.
-------------- next part --------------
From d7c898b8104710b2de92ec7233ef34c2c846fe22 Mon Sep 17 00:00:00 2001
From: Greg Fitzgerald <gregf at codeaurora.org>
Date: Mon, 24 Sep 2012 16:24:05 -0700
Subject: [PATCH] Bug 13039: Fixed branch encoding in Thumb2 code generation

http://llvm.org/bugs/show_bug.cgi?id=13039
---
 lib/Target/ARM/ARMInstrInfo.td                   |    1 +
 lib/Target/ARM/ARMInstrThumb2.td                 |   13 +++----
 lib/Target/ARM/Disassembler/ARMDisassembler.cpp  |   39 ++++++++++++---------
 lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp    |   18 ++++-----
 lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp |   30 +++++++++--------
 test/MC/ARM/basic-thumb2-instructions.s          |   14 ++++++++
 test/MC/Disassembler/ARM/thumb2.txt              |    2 +
 7 files changed, 69 insertions(+), 48 deletions(-)

diff --git a/lib/Target/ARM/ARMInstrInfo.td b/lib/Target/ARM/ARMInstrInfo.td
index 20d7c1b..a811d50 100644
--- a/lib/Target/ARM/ARMInstrInfo.td
+++ b/lib/Target/ARM/ARMInstrInfo.td
@@ -364,6 +364,7 @@ def brtarget : Operand<OtherVT> {
 // FIXME: get rid of this one?
 def uncondbrtarget : Operand<OtherVT> {
   let EncoderMethod = "getUnconditionalBranchTargetOpValue";
+  let DecoderMethod = "DecodeT2UncondBROperand";
   let OperandType = "OPERAND_PCREL";
 }
 
diff --git a/lib/Target/ARM/ARMInstrThumb2.td b/lib/Target/ARM/ARMInstrThumb2.td
index f1a6cce..81bb6c8 100644
--- a/lib/Target/ARM/ARMInstrThumb2.td
+++ b/lib/Target/ARM/ARMInstrThumb2.td
@@ -3239,17 +3239,16 @@ let isPredicable = 1 in
 def t2B   : T2I<(outs), (ins uncondbrtarget:$target), IIC_Br,
                  "b", ".w\t$target",
                  [(br bb:$target)]> {
+
+  bits<24> target;
   let Inst{31-27} = 0b11110;
+  let Inst{26} = target{23};
+  let Inst{25-16} = target{20-11};
   let Inst{15-14} = 0b10;
+  let Inst{13} = target{22};
   let Inst{12} = 1;
-
-  bits<20> target;
-  let Inst{26} = target{19};
-  let Inst{11} = target{18};
-  let Inst{13} = target{17};
-  let Inst{21-16} = target{16-11};
+  let Inst{11} = target{21};
   let Inst{10-0} = target{10-0};
-  let DecoderMethod = "DecodeT2BInstruction";
 }
 
 let isNotDuplicable = 1, isIndirectBranch = 1 in {
diff --git a/lib/Target/ARM/Disassembler/ARMDisassembler.cpp b/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
index bf0dabb..3af1c55 100644
--- a/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
+++ b/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
@@ -237,8 +237,6 @@ static DecodeStatus DecodeAddrMode5Operand(MCInst &Inst, unsigned Val,
                                uint64_t Address, const void *Decoder);
 static DecodeStatus DecodeAddrMode7Operand(MCInst &Inst, unsigned Val,
                                uint64_t Address, const void *Decoder);
-static DecodeStatus DecodeT2BInstruction(MCInst &Inst, unsigned Insn,
-                               uint64_t Address, const void *Decoder);
 static DecodeStatus DecodeBranchImmInstruction(MCInst &Inst,unsigned Insn,
                                uint64_t Address, const void *Decoder);
 static DecodeStatus DecodeAddrMode6Operand(MCInst &Inst, unsigned Val,
@@ -323,6 +321,8 @@ static DecodeStatus DecodeThumbBROperand(MCInst &Inst, unsigned Val,
                                uint64_t Address, const void *Decoder);
 static DecodeStatus DecodeT2BROperand(MCInst &Inst, unsigned Val,
                                uint64_t Address, const void *Decoder);
+static DecodeStatus DecodeT2UncondBROperand(MCInst &Inst, unsigned Val,
+                               uint64_t Address, const void *Decoder);
 static DecodeStatus DecodeThumbCmpBROperand(MCInst &Inst, unsigned Val,
                                uint64_t Address, const void *Decoder);
 static DecodeStatus DecodeThumbAddrModeRR(MCInst &Inst, unsigned Val,
@@ -2092,21 +2092,6 @@ static DecodeStatus DecodeAddrMode7Operand(MCInst &Inst, unsigned Val,
 }
 
 static DecodeStatus
-DecodeT2BInstruction(MCInst &Inst, unsigned Insn,
-                     uint64_t Address, const void *Decoder) {
-  DecodeStatus S = MCDisassembler::Success;
-  unsigned imm = (fieldFromInstruction(Insn, 0, 11) << 0) |
-                 (fieldFromInstruction(Insn, 11, 1) << 18) |
-                 (fieldFromInstruction(Insn, 13, 1) << 17) |
-                 (fieldFromInstruction(Insn, 16, 6) << 11) |
-                 (fieldFromInstruction(Insn, 26, 1) << 19);
-  if (!tryAddingSymbolicOperand(Address, Address + SignExtend32<20>(imm<<1) + 4,
-                                true, 4, Inst, Decoder))
-    Inst.addOperand(MCOperand::CreateImm(SignExtend32<20>(imm << 1)));
-  return S;
-}
-
-static DecodeStatus
 DecodeBranchImmInstruction(MCInst &Inst, unsigned Insn,
                            uint64_t Address, const void *Decoder) {
   DecodeStatus S = MCDisassembler::Success;
@@ -3044,6 +3029,26 @@ static DecodeStatus DecodeT2BROperand(MCInst &Inst, unsigned Val,
   return MCDisassembler::Success;
 }
 
+static DecodeStatus DecodeT2UncondBROperand(MCInst &Inst, unsigned Val,
+                                 uint64_t Address, const void *Decoder) {
+  bool S  = Val & 0x800000;
+  bool J1 = Val & 0x400000;
+  bool J2 = Val & 0x200000;
+
+  bool I1 = !(J1 ^ S);
+  bool I2 = !(J2 ^ S);
+
+  Val &= ~0x600000; // Clear I1 and I2 bits
+  Val |= I1 ? 0x400000 : 0;
+  Val |= I2 ? 0x200000 : 0;
+
+  if (!tryAddingSymbolicOperand(Address, Address + SignExtend32<25>(Val<<1) + 4,
+                                true, 4, Inst, Decoder))
+    Inst.addOperand(MCOperand::CreateImm(SignExtend32<25>(Val << 1)));
+
+  return MCDisassembler::Success;
+}
+
 static DecodeStatus DecodeThumbCmpBROperand(MCInst &Inst, unsigned Val,
                                  uint64_t Address, const void *Decoder) {
   if (!tryAddingSymbolicOperand(Address, Address + SignExtend32<7>(Val<<1) + 4,
diff --git a/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp b/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
index 68c47ac..79198c4 100644
--- a/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
+++ b/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
@@ -362,16 +362,14 @@ static unsigned adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
     Value >>= 1; // Low bit is not encoded.
 
     uint32_t out = 0;
-    bool I =  Value & 0x800000;
-    bool J1 = Value & 0x400000;
-    bool J2 = Value & 0x200000;
-    J1 ^= I;
-    J2 ^= I;
-
-    out |= I  << 26; // S bit
-    out |= !J1 << 13; // J1 bit
-    out |= !J2 << 11; // J2 bit
-    out |= (Value & 0x1FF800)  << 5; // imm6 field
+    bool S =  Value & 0x800000;
+    bool I1 = Value & 0x400000;
+    bool I2 = Value & 0x200000;
+
+    out |= S  << 26; // S bit
+    out |= !(I1 ^ S) << 13; // J1 bit
+    out |= !(I2 ^ S) << 11; // J2 bit
+    out |= (Value & 0x1FF800)  << 5;  // imm10 field
     out |= (Value & 0x0007FF);        // imm11 field
 
     uint64_t swapped = (out & 0xFFFF0000) >> 16;
diff --git a/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp b/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
index d0e127a..69a4d83 100644
--- a/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
+++ b/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
@@ -625,20 +625,22 @@ getARMBLXTargetOpValue(const MCInst &MI, unsigned OpIdx,
 uint32_t ARMMCCodeEmitter::
 getUnconditionalBranchTargetOpValue(const MCInst &MI, unsigned OpIdx,
                        SmallVectorImpl<MCFixup> &Fixups) const {
-  unsigned Val =
-    ::getBranchTargetOpValue(MI, OpIdx, ARM::fixup_t2_uncondbranch, Fixups);
-  bool I  = (Val & 0x800000);
-  bool J1 = (Val & 0x400000);
-  bool J2 = (Val & 0x200000);
-  if (I ^ J1)
-    Val &= ~0x400000;
-  else
-    Val |= 0x400000;
+  const MCOperand MO = MI.getOperand(OpIdx);
+  if (MO.isExpr())
+    return ::getBranchTargetOpValue(MI, OpIdx, ARM::fixup_t2_uncondbranch, Fixups);
 
-  if (I ^ J2)
-    Val &= ~0x200000;
-  else
-    Val |= 0x200000;
+  uint32_t Val = MO.getImm() >> 1;
+
+  bool S  = Val & 0x800000;
+  bool I1 = Val & 0x400000;
+  bool I2 = Val & 0x200000;
+
+  bool J1 = !(I1 ^ S);
+  bool J2 = !(I2 ^ S);
+
+  Val &= ~0x600000; // Clear J1 and J2 bits
+  Val |= J1 ? 0x400000 : 0;
+  Val |= J2 ? 0x200000 : 0;
 
   return Val;
 }
diff --git a/test/MC/ARM/basic-thumb2-instructions.s b/test/MC/ARM/basic-thumb2-instructions.s
index 23d9f59..c593154 100644
--- a/test/MC/ARM/basic-thumb2-instructions.s
+++ b/test/MC/ARM/basic-thumb2-instructions.s
@@ -211,12 +211,26 @@ _func:
 @------------------------------------------------------------------------------
 @ B
 @------------------------------------------------------------------------------
+        b.w     #4194304   @ !S !I1  I2
+        b.w     #8388608   @ !S  I1 !I2
+        b.w     #12582912  @ !S  I1  I2
+        b.w     #16777216  @  S !I1 !I2
+        b.w     #20971520  @  S !I1  I2
+        b.w     #25165824  @  S  I1 !I2
+        b.w     #29360128  @  S  I1  I2
         b.w   _bar
         beq.w   _bar
         it eq
         beq.w _bar
         bmi.w   #-183396
 
+@ CHECK: b.w #4194304                    @ encoding: [0x00,0xf0,0x00,0xb0]
+@ CHECK: b.w #8388608                    @ encoding: [0x00,0xf0,0x00,0x98]
+@ CHECK: b.w #12582912                   @ encoding: [0x00,0xf0,0x00,0x90]
+@ CHECK: b.w #16777216                   @ encoding: [0x00,0xf4,0x00,0x90]
+@ CHECK: b.w #20971520                   @ encoding: [0x00,0xf4,0x00,0x98]
+@ CHECK: b.w #25165824                   @ encoding: [0x00,0xf4,0x00,0xb0]
+@ CHECK: b.w #29360128                   @ encoding: [0x00,0xf4,0x00,0xb8]
 @ CHECK: b.w	_bar                    @ encoding: [A,0xf0'A',A,0x90'A']
           @   fixup A - offset: 0, value: _bar, kind: fixup_t2_uncondbranch
 @ CHECK: beq.w	_bar                    @ encoding: [A,0xf0'A',A,0x80'A']
diff --git a/test/MC/Disassembler/ARM/thumb2.txt b/test/MC/Disassembler/ARM/thumb2.txt
index 42ebe58..c866321 100644
--- a/test/MC/Disassembler/ARM/thumb2.txt
+++ b/test/MC/Disassembler/ARM/thumb2.txt
@@ -166,8 +166,10 @@
 # B
 #------------------------------------------------------------------------------
 # CHECK: bmi.w   #-183396
+# CHECK: b.w     #4194304
 
 0x13 0xf5 0xce 0xa9
+0x00 0xf0 0x00 0xb0
 
 
 #------------------------------------------------------------------------------
-- 
1.7.5.4



More information about the llvm-commits mailing list