[llvm-commits] [PATCH]Add support for fstmx/fldmx instructions on the ARM backend
Silviu Baranga
silbar01 at arm.com
Mon Jun 25 09:46:07 PDT 2012
Hi Jim,
Thanks for the review. I will send another updated patch shortly.
Regards,
Silviu
From: Jim Grosbach [mailto:grosbach at apple.com]
Sent: 22 June 2012 20:43
To: Silviu Baranga
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH]Add support for fstmx/fldmx instructions on the ARM
backend
Hi Silviu
OK. More detailed comments on the specifics below.
On May 24, 2012, at 5:21 AM, Silviu Baranga <silbar01 at arm.com> wrote:
Hi Jim,
Thanks for the review.
These instructions also support the "DB" suffix, and having
either IA/DB is a required part of the syntax.
The tests from fstmx-arm.txt were supposed to test that
some invalid bit patterns are not decoded as vstm/vldm
instructions (which is the current behaviour).
Those bit patterns are invalid because they have an odd
imm8 field (so they are not vstm/vldm instructions) and
the value of the 22nd bit is 1 (so they are not fstmx/fldmx
instructions).
I've removed the fstmx-arm.txt test, and added instead
the invalid-fstmx.txt test file (the new patch is attached).
Index: test/MC/ARM/simple-fp-encoding.s
===================================================================
--- test/MC/ARM/simple-fp-encoding.s (revision 156615)
+++ test/MC/ARM/simple-fp-encoding.s (working copy)
@@ -352,3 +352,12 @@
@ CHECK: vmov.i32 d4, #0x0 @ encoding:
[0x10,0x40,0x80,0xf2]
@ CHECK: vmov.i32 d4, #0x42000000 @ encoding:
[0x12,0x46,0x84,0xf2]
+
+ fldmiaxeq r0, {d4,d5}
+ fstmiaxeq r4, {d8,d9}
+ fldmdbxne r5!, {d4,d5,d6}
+ fstmdbxne r7!, {d2-d4}
+@ CHECK: fldmiaxeq r0, {d4-d5} @ encoding: [0x05,0x4b,0x90,0x0c]
+@ CHECK: fstmiaxeq r4, {d8-d9} @ encoding: [0x05,0x8b,0x84,0x0c]
+@ CHECK: fldmdbxne r5!, {d4-d6} @ encoding: [0x07,0x4b,0x35,0x1d]
+@ CHECK: fstmdbxne r7!, {d2-d4} @ encoding: [0x07,0x2b,0x27,0x1d]
Index: test/MC/Disassembler/ARM/invalid-fstmx.txt
===================================================================
--- test/MC/Disassembler/ARM/invalid-fstmx.txt (revision 0)
+++ test/MC/Disassembler/ARM/invalid-fstmx.txt (revision 0)
@@ -0,0 +1,4 @@
+# RUN: llvm-mc --disassemble %s -triple=armv7-linux-gnueabi |& FileCheck %s
+
+0x0d 0x0b 0xc7 0x0c
+# CHECK: invalid instruction encoding
Index: test/MC/Disassembler/ARM/fp-encoding.txt
===================================================================
--- test/MC/Disassembler/ARM/fp-encoding.txt (revision 156615)
+++ test/MC/Disassembler/ARM/fp-encoding.txt (working copy)
@@ -238,3 +238,16 @@
# CHECK: vcvtr.s32.f32 s0, s1
# CHECK: vcvtr.u32.f64 s0, d0
# CHECK: vcvtr.u32.f32 s0, s1
+
+0x0f 0x3b 0xb7 0x0c
+0x0d 0x4b 0x96 0x0c
+0x09 0x1b 0x38 0xed
+0x07 0x2b 0x83 0xec
+0x05 0x5b 0xa3 0x0c
+0x0f 0x3b 0x20 0x1d
+# CHECK: fldmiaxeq r7!, {d3-d9}
+# CHECK: fldmiaxeq r6, {d4-d9}
+# CHECK: fldmdbx r8!, {d1-d4}
+# CHECK: fstmiax r3, {d2-d4}
+# CHECK: fstmiaxeq r3!, {d5-d6}
+# CHECK: fstmdbxne r0!, {d3-d9}
Index: utils/TableGen/EDEmitter.cpp
===================================================================
--- utils/TableGen/EDEmitter.cpp (revision 156615)
+++ utils/TableGen/EDEmitter.cpp (working copy)
@@ -700,6 +700,7 @@
MISC("addr_offset_none", "kOperandTypeARMAddrMode7"); // R
MISC("reglist", "kOperandTypeARMRegisterList"); // I, R,
...
MISC("dpr_reglist", "kOperandTypeARMDPRRegisterList"); // I, R,
...
+ MISC("dpr_reglist_fmx", "kOperandTypeARMDPRRegisterList"); // I, R,
...
MISC("spr_reglist", "kOperandTypeARMSPRRegisterList"); // I, R,
...
MISC("it_mask", "kOperandTypeThumbITMask"); // I
MISC("t2addrmode_reg", "kOperandTypeThumb2AddrModeReg"); // R
Index: lib/Target/ARM/AsmParser/ARMAsmParser.cpp
===================================================================
--- lib/Target/ARM/AsmParser/ARMAsmParser.cpp (revision 156615)
+++ lib/Target/ARM/AsmParser/ARMAsmParser.cpp (working copy)
@@ -849,6 +849,26 @@
bool isReg() const { return Kind == k_Register; }
bool isRegList() const { return Kind == k_RegisterList; }
bool isDPRRegList() const { return Kind == k_DPRRegisterList; }
+ bool isDPRRegListFMX() const {
+ // We need to check that we don't use a D reg with an index greater
+ // then 15.
+ if (Kind != k_DPRRegisterList)
+ return false;
+ const SmallVectorImpl<unsigned> &list = getRegList();
+ for (SmallVectorImpl<unsigned>::const_iterator I = list.begin(),
+ E = list.end();
+ I != E;
+ ++I){
+ // We only allow D registers smaller then 16.
+ if (!(ARMMCRegisterClasses[ARM::DPRRegClassID].contains(*I)))
+ return false;
+ if (getARMRegisterNumbering(*I) > 15) return false;
We have the DPR_VFP2 register class for the low 16 DPR registers. You can
just update the .contains() check instead.
Relatedly, the name of the operand type shouldn't reflect the instructions,
but the nature of the registers. Since this is a list of DPR_VFP2 registers,
"DPR_VFP2RegList" seems to make a certain amount of sense.
+ }
+ // No registers were transfered.
"transferred". That said, I'm not sure what this comment means. No
transference is occurring here.
+ if (list.size() == 0) return false;
This can be an assert() at the top of the function if it's here at all. An
empty register list should result in an error in parsing and never be
created as an asmoperand.
+
+ return true;
+ }
bool isSPRRegList() const { return Kind == k_SPRRegisterList; }
bool isToken() const { return Kind == k_Token; }
bool isMemBarrierOpt() const { return Kind == k_MemBarrierOpt; }
@@ -1472,6 +1492,16 @@
addRegListOperands(Inst, N);
}
+ void addDPRRegListFMXOperands(MCInst &Inst, unsigned N) const {
+ const SmallVectorImpl<unsigned> &RegList = getRegList();
+ assert(RegList.size() > 0 && "Register list size must be greater then
0.");
+
+ // Add the operand indicating the first register.
+ Inst.addOperand(MCOperand::CreateReg(RegList[0]));
+ // Add the immediate operand indicating the number of registers used.
+ Inst.addOperand(MCOperand::CreateImm(RegList.size()));
+ }
+
Why is this represented differently than the existing register lists for
vldm/vstm? That adds a lot of unnecessary complexity to the patch. Just use
the existing infrastructure.
void addSPRRegListOperands(MCInst &Inst, unsigned N) const {
addRegListOperands(Inst, N);
}
Index: lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp
===================================================================
--- lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp (revision
156615)
+++ lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp (working copy)
@@ -603,6 +603,27 @@
O << ", asr #" << Imm;
}
+void ARMInstPrinter::printRegisterFMXList(const MCInst *MI, unsigned OpNum,
+ raw_ostream &O) {
+ unsigned BaseReg = MI->getOperand(OpNum).getReg();
+ unsigned NumRegs = MI->getOperand(OpNum + 1).getImm();
+
+ O << "{";
+ // We will write this with the - format, since the upper registers may
not
+ // exist.
+ switch(NumRegs){
+ case 0:
+ break;
+ case 1:
+ O << getRegisterName(BaseReg);
+ break;
+ default:
+ O << getRegisterName(BaseReg);
+ O << "-d" << getARMRegisterNumbering(BaseReg) + NumRegs - 1;
+ }
+ O << "}";
+}
+
This can go away and the operand can just reference the existing
printRegisterList().
void ARMInstPrinter::printRegisterList(const MCInst *MI, unsigned OpNum,
raw_ostream &O) {
O << "{";
Index: lib/Target/ARM/InstPrinter/ARMInstPrinter.h
===================================================================
--- lib/Target/ARM/InstPrinter/ARMInstPrinter.h (revision 156615)
+++ lib/Target/ARM/InstPrinter/ARMInstPrinter.h (working copy)
@@ -114,6 +114,8 @@
raw_ostream &O);
void printSBitModifierOperand(const MCInst *MI, unsigned OpNum,
raw_ostream &O);
+ void printRegisterFMXList(const MCInst *MI, unsigned OpNum,
+ raw_ostream &O);
void printRegisterList(const MCInst *MI, unsigned OpNum, raw_ostream &O);
void printNoHashImmediate(const MCInst *MI, unsigned OpNum, raw_ostream
&O);
void printPImmediate(const MCInst *MI, unsigned OpNum, raw_ostream &O);
Index: lib/Target/ARM/ARMInstrVFP.td
===================================================================
--- lib/Target/ARM/ARMInstrVFP.td (revision 156615)
+++ lib/Target/ARM/ARMInstrVFP.td (working copy)
@@ -118,7 +118,7 @@
InstrItinClass itin, InstrItinClass itin_upd> {
// Double Precision
def DIA :
- AXDI4<(outs), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs, variable_ops),
+ AXDI4_D<(outs), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs,
variable_ops),
IndexModeNone, itin,
!strconcat(asm, "ia${p}\t$Rn, $regs"), "", []> {
let Inst{24-23} = 0b01; // Increment After
@@ -126,7 +126,7 @@
let Inst{20} = L_bit;
}
def DIA_UPD :
- AXDI4<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs,
+ AXDI4_D<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs,
variable_ops),
IndexModeUpd, itin_upd,
!strconcat(asm, "ia${p}\t$Rn!, $regs"), "$Rn = $wb", []> {
@@ -135,7 +135,7 @@
let Inst{20} = L_bit;
}
def DDB_UPD :
- AXDI4<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs,
+ AXDI4_D<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs,
variable_ops),
IndexModeUpd, itin_upd,
!strconcat(asm, "db${p}\t$Rn!, $regs"), "$Rn = $wb", []> {
@@ -216,7 +216,41 @@
(VLDMDIA_UPD SP, pred:$p, dpr_reglist:$r)>;
// FLDMX, FSTMX - mixing S/D registers for pre-armv6 cores
+// These instructions are deprecated, so we don't want them to get
selected.
This comment confuses me. Are these instructions able to reference the S
registers as well as the D registers? Nothing in this patch handles that, as
far as I can tell. Everything talks about D registers.
+multiclass vfp_fldst_mult<string asm, bit L_bit,
+ InstrItinClass itin, InstrItinClass itin_upd> {
+ // Double Precision
+ def DIA :
+ AXDI4_F<(outs), (ins GPR:$Rn, pred:$p, dpr_reglist_fmx:$regs,
variable_ops),
+ IndexModeNone, itin,
+ !strconcat(asm, "iax${p}\t$Rn, $regs"), "", []> {
+ let Inst{24-23} = 0b01; // Increment After
+ let Inst{21} = 0; // No writeback
+ let Inst{20} = L_bit;
+ }
+ def DIA_UPD :
+ AXDI4_F<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist_fmx:$regs,
+ variable_ops),
+ IndexModeUpd, itin_upd,
+ !strconcat(asm, "iax${p}\t$Rn!, $regs"), "$Rn = $wb", []> {
+ let Inst{24-23} = 0b01; // Increment After
+ let Inst{21} = 1; // Writeback
+ let Inst{20} = L_bit;
+ }
+ def DDB_UPD :
+ AXDI4_F<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist_fmx:$regs,
+ variable_ops),
+ IndexModeUpd, itin_upd,
+ !strconcat(asm, "dbx${p}\t$Rn!, $regs"), "$Rn = $wb", []> {
+ let Inst{24-23} = 0b10; // Decrement Before
+ let Inst{21} = 1; // Writeback
+ let Inst{20} = L_bit;
+ }
+}
+
+defm FLDM : vfp_fldst_mult<"fldm", 1, NoItinerary, NoItinerary>;
+defm FSTM : vfp_fldst_mult<"fstm", 0, NoItinerary, NoItinerary>;
//===----------------------------------------------------------------------=
==//
// FP Binary Operations.
//
Index: lib/Target/ARM/ARMInstrInfo.td
===================================================================
--- lib/Target/ARM/ARMInstrInfo.td (revision 156615)
+++ lib/Target/ARM/ARMInstrInfo.td (working copy)
@@ -386,6 +386,14 @@
let DecoderMethod = "DecodeDPRRegListOperand";
}
+def DPRRegListFMXAsmOperand : AsmOperandClass {let Name = "DPRRegListFMX";
}
+def dpr_reglist_fmx : Operand<i32> {
+ let EncoderMethod = "getRegisterFMXListOpValue";
+ let ParserMatchClass = DPRRegListFMXAsmOperand;
+ let PrintMethod = "printRegisterFMXList";
+ let DecoderMethod = "DecodeDPRRegFMXListOperand";
Other than the AsmOperandClass, these can all refer to the same functions as
for the normal DPRRegList.
+}
+
def SPRRegListAsmOperand : AsmOperandClass { let Name = "SPRRegList"; }
def spr_reglist : Operand<i32> {
let EncoderMethod = "getRegisterListOpValue";
Index: lib/Target/ARM/ARMCodeEmitter.cpp
===================================================================
--- lib/Target/ARM/ARMCodeEmitter.cpp (revision 156615)
+++ lib/Target/ARM/ARMCodeEmitter.cpp (working copy)
@@ -320,6 +320,8 @@
unsigned getNEONVcvtImm32OpValue(const MachineInstr &MI, unsigned Op)
const { return 0; }
+ unsigned getRegisterFMXListOpValue(const MachineInstr &MI, unsigned Op)
+ const { return 0; }
unsigned getRegisterListOpValue(const MachineInstr &MI, unsigned Op)
const { return 0; }
Index: lib/Target/ARM/ARMInstrFormats.td
===================================================================
--- lib/Target/ARM/ARMInstrFormats.td (revision 156615)
+++ lib/Target/ARM/ARMInstrFormats.td (working copy)
@@ -1426,16 +1426,32 @@
// Encode instruction operands.
let Inst{19-16} = Rn;
- let Inst{22} = regs{12};
let Inst{15-12} = regs{11-8};
- let Inst{7-0} = regs{7-0};
-
+ let Inst{7-1} = regs{7-1};
Where do the bits being removed here get set for the old instructions from
the vfp_ldst_mult multiclass?
// TODO: Mark the instructions with the appropriate subtarget info.
let Inst{27-25} = 0b110;
let Inst{11-9} = 0b101;
let Inst{8} = 1; // Double precision
+
+ let regs{0} = 0;
What is this supposed to be doing? We shouldn't be changing the value of
regs here. Operand values should be considered to be const.
}
+class AXDI4_D<dag oops, dag iops, IndexMode im, InstrItinClass itin,
+ string asm, string cstr, list<dag> pattern>
+ : AXDI4<oops, iops, im, itin, asm, cstr, pattern> {
+ let Inst{22} = regs{12};
+ let Inst{12} = regs{8};
Inst{12} is still set in the base class. Why set it again here?
+ let Inst{0} = 0;
+}
+
+class AXDI4_F<dag oops, dag iops, IndexMode im, InstrItinClass itin,
+ string asm, string cstr, list<dag> pattern>
+ : AXDI4<oops, iops, im, itin, asm, cstr, pattern> {
+ let Inst{22} = 0;
+ let Inst{0} = 1;
+ let regs{12} = 0;
+}
+
class AXSI4<dag oops, dag iops, IndexMode im, InstrItinClass itin,
string asm, string cstr, list<dag> pattern>
: VFPXI<oops, iops, AddrMode4, 4, im,
Index: lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
===================================================================
--- lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp (revision 156615)
+++ lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp (working copy)
@@ -283,7 +283,9 @@
unsigned getBitfieldInvertedMaskOpValue(const MCInst &MI, unsigned Op,
SmallVectorImpl<MCFixup> &Fixups)
const;
-
+
+ unsigned getRegisterFMXListOpValue(const MCInst &MI, unsigned Op,
+ SmallVectorImpl<MCFixup> &Fixups)
const;
unsigned getRegisterListOpValue(const MCInst &MI, unsigned Op,
SmallVectorImpl<MCFixup> &Fixups) const;
unsigned getAddrMode6AddressOpValue(const MCInst &MI, unsigned Op,
@@ -1335,6 +1337,21 @@
return lsb | (msb << 5);
}
+unsigned ARMMCCodeEmitter:: getRegisterFMXListOpValue(const MCInst &MI,
+ unsigned Op, SmallVectorImpl<MCFixup> &Fixups) const
{
+
+ unsigned Reg = MI.getOperand(Op).getReg();
+ unsigned NumRegs = MI.getOperand(Op + 1).getImm();
+
+ unsigned RegNo = getARMRegisterNumbering(Reg);
+ unsigned Binary = 0;
+
+ Binary |= (RegNo & 0x1f) << 8;
+ Binary |= NumRegs * 2;
+
+ return Binary;
+}
+
This can just use the encoder already present for DPR lists. No need for a
new one.
unsigned ARMMCCodeEmitter::
getRegisterListOpValue(const MCInst &MI, unsigned Op,
SmallVectorImpl<MCFixup> &Fixups) const {
Index: lib/Target/ARM/Disassembler/ARMDisassembler.cpp
===================================================================
--- lib/Target/ARM/Disassembler/ARMDisassembler.cpp (revision 156615)
+++ lib/Target/ARM/Disassembler/ARMDisassembler.cpp (working copy)
@@ -197,7 +197,10 @@
uint64_t Address, const void *Decoder);
static DecodeStatus DecodeDPRRegListOperand(MCInst &Inst, unsigned Val,
uint64_t Address, const void *Decoder);
+static DecodeStatus DecodeDPRRegFMXListOperand(MCInst &Inst, unsigned Val,
+ uint64_t Address, const void *Decoder);
+
static DecodeStatus DecodeBitfieldMaskOperand(MCInst &Inst, unsigned Insn,
uint64_t Address, const void *Decoder);
static DecodeStatus DecodeCopMemInstruction(MCInst &Inst, unsigned Insn,
@@ -1237,18 +1240,38 @@
return S;
}
+static DecodeStatus DecodeDPRRegFMXListOperand(MCInst &Inst, unsigned Val,
+ uint64_t Address, const void *Decoder){
+ DecodeStatus S = MCDisassembler::Success;
+
+ unsigned Vd = fieldFromInstruction32(Val, 8, 4);
+ int regs = fieldFromInstruction32(Val, 1, 7);
+
+ if (regs == 0 || Vd + regs > 16)
+ S = MCDisassembler::SoftFail;
+
+ if (!Check(S, DecodeDPRRegisterClass(Inst, Vd, Address, Decoder)))
+ return MCDisassembler::Fail;
+
+ Inst.addOperand(MCOperand::CreateImm(regs));
+
+ return S;
+}
+
Likewise.
static DecodeStatus DecodeDPRRegListOperand(MCInst &Inst, unsigned Val,
uint64_t Address, const void *Decoder) {
DecodeStatus S = MCDisassembler::Success;
unsigned Vd = fieldFromInstruction32(Val, 8, 5);
- unsigned regs = fieldFromInstruction32(Val, 0, 8);
+ int regs = fieldFromInstruction32(Val, 1, 7);
- regs = regs >> 1;
+ if (regs == 0 || regs > 16 || Vd+regs > 32)
+ S = MCDisassembler::SoftFail;
if (!Check(S, DecodeDPRRegisterClass(Inst, Vd, Address, Decoder)))
return MCDisassembler::Fail;
- for (unsigned i = 0; i < (regs - 1); ++i) {
+
+ for (int i = 0; i < (regs - 1); ++i) {
if (!Check(S, DecodeDPRRegisterClass(Inst, ++Vd, Address, Decoder)))
return MCDisassembler::Fail;
}
Thanks,
Silviu
From: Jim Grosbach [mailto:grosbach at apple.com]
Sent: 22 May 2012 17:32
To: Silviu Baranga
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH]Add support for fstmx/fldmx instructions on the ARM
backend
Hi Silviu,
Do these instructions only support the "IA" suffix? Is that suffix always a
required part of the syntax?
Index: test/MC/Disassembler/ARM/fstmx-arm.txt
===================================================================
--- test/MC/Disassembler/ARM/fstmx-arm.txt (revision 0)
+++ test/MC/Disassembler/ARM/fstmx-arm.txt (revision 0)
@@ -0,0 +1,9 @@
+# RUN: echo "0x0d 0x0b 0xc7 0x0c" | llvm-mc --disassemble
-triple=armv7-linux-gnueabi |& FileCheck %s
+# RUN: echo "0x0b 0x5b 0xd2 0x0c" | llvm-mc --disassemble
-triple=armv7-linux-gnueabi |& FileCheck %s
+# RUN: echo "0x07 0x1b 0x69 0x0d" | llvm-mc --disassemble
-triple=armv7-linux-gnueabi |& FileCheck %s
+# RUN: echo "0x09 0xeb 0x37 0x0d" | llvm-mc --disassemble
-triple=armv7-linux-gnueabi |& FileCheck %s
+# RUN: echo "0x0d 0xfb 0xa4 0x0c" | llvm-mc --disassemble
-triple=armv7-linux-gnueabi |& FileCheck %s
+# RUN: echo "0x0b 0x3b 0xfb 0x0c" | llvm-mc --disassemble
-triple=armv7-linux-gnueabi |& FileCheck %s
+
+# CHECK-NOT: vstm
+# CHECK-NOT: vldm
Why all the separate run lines with 'echo' instead of having the bytes in
the file directly, one instruction per line, like the other tests?
Additionally, this should be checking what the disassembly should be, not
just what it should not be. i.e., CHECK lines as well as CHECK-NOT.
Relatedly, what is this testing that the entries in fl-encoding.txt do not
cover?
-Jim
On May 22, 2012, at 4:05 AM, Silviu Baranga < <mailto:silbar01 at arm.com>
silbar01 at arm.com> wrote:
Hi,
The fstmx/fldmx ARM instructions are currently not implemented in the ARM
backend.
Although these instructions are deprecated, they should still be supported
for assembly and
disassembly purposes.
The patch adds support for these instructions and also fixes a problem with
the
vstm/vldm instructions which were colliding with the fstmx/fldmx encoding
space.
Please review this patch.
Thanks,
Silviu
<fstmx.diff>
<fstmx.diff>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120625/eacd634d/attachment.html>
More information about the llvm-commits
mailing list