[llvm-commits] [PATCH] thumb t2b fix
Greg Fitzgerald
garious at gmail.com
Mon Sep 24 17:15:28 PDT 2012
> 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 a5584514e8bee7170c8d25889001cccd45af87b2 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 | 37 ++++++++++++----------
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, 67 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..c9b254d 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,
@@ -2092,21 +2090,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 +3027,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