[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