<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><base href="x-msg://18157/"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Silviu<div><br></div><div>OK. More detailed comments on the specifics below.</div><div><br></div><div><br><div><div>On May 24, 2012, at 5:21 AM, Silviu Baranga <<a href="mailto:silbar01@arm.com">silbar01@arm.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div lang="EN-GB" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div class="Section1" style="page: Section1; "><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Hi Jim,<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Thanks for the review.<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">These instructions also support the “DB” suffix, and having<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">either IA/DB is a required part of the syntax.<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">The tests from fstmx-arm.txt were supposed to test that<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">some invalid bit patterns are not decoded as vstm/vldm<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">instructions (which is the current behaviour).<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Those bit patterns are invalid because they have an odd<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">imm8 field (so they are not vstm/vldm instructions) and<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">the value of the 22nd bit is 1 (so they are not fstmx/fldmx<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">instructions).<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">I’ve removed the fstmx-arm.txt test, and added instead<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">the invalid-fstmx.txt test file (the new patch is attached).</span></div></div></div></blockquote><div><br></div><div><blockquote type="cite"><div>Index: test/MC/ARM/simple-fp-encoding.s</div><div>===================================================================</div><div>--- test/MC/ARM/simple-fp-encoding.s<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ test/MC/ARM/simple-fp-encoding.s<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -352,3 +352,12 @@</div><div> </div><div> @ CHECK: vmov.i32<span class="Apple-tab-span" style="white-space:pre"> </span>d4, #0x0 @ encoding: [0x10,0x40,0x80,0xf2]</div><div> @ CHECK: vmov.i32<span class="Apple-tab-span" style="white-space:pre"> </span>d4, #0x42000000 @ encoding: [0x12,0x46,0x84,0xf2]</div><div>+</div><div>+ fldmiaxeq r0, {d4,d5}</div><div>+ fstmiaxeq r4, {d8,d9}</div><div>+ fldmdbxne r5!, {d4,d5,d6}</div><div>+ fstmdbxne r7!, {d2-d4}</div><div>+@ CHECK: fldmiaxeq r0, {d4-d5} @ encoding: [0x05,0x4b,0x90,0x0c]</div><div>+@ CHECK: fstmiaxeq r4, {d8-d9} @ encoding: [0x05,0x8b,0x84,0x0c]</div><div>+@ CHECK: fldmdbxne r5!, {d4-d6} @ encoding: [0x07,0x4b,0x35,0x1d]</div><div>+@ CHECK: fstmdbxne r7!, {d2-d4} @ encoding: [0x07,0x2b,0x27,0x1d]</div><div>Index: test/MC/Disassembler/ARM/invalid-fstmx.txt</div><div>===================================================================</div><div>--- test/MC/Disassembler/ARM/invalid-fstmx.txt<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div><div>+++ test/MC/Disassembler/ARM/invalid-fstmx.txt<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div><div>@@ -0,0 +1,4 @@</div><div>+# RUN: llvm-mc --disassemble %s -triple=armv7-linux-gnueabi |& FileCheck %s</div><div>+</div><div>+0x0d 0x0b 0xc7 0x0c</div><div>+# CHECK: invalid instruction encoding</div><div>Index: test/MC/Disassembler/ARM/fp-encoding.txt</div><div>===================================================================</div><div>--- test/MC/Disassembler/ARM/fp-encoding.txt<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ test/MC/Disassembler/ARM/fp-encoding.txt<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -238,3 +238,16 @@</div><div> # CHECK: vcvtr.s32.f32 s0, s1</div><div> # CHECK: vcvtr.u32.f64 s0, d0</div><div> # CHECK: vcvtr.u32.f32 s0, s1</div><div>+</div><div>+0x0f 0x3b 0xb7 0x0c</div><div>+0x0d 0x4b 0x96 0x0c</div><div>+0x09 0x1b 0x38 0xed</div><div>+0x07 0x2b 0x83 0xec</div><div>+0x05 0x5b 0xa3 0x0c</div><div>+0x0f 0x3b 0x20 0x1d</div><div>+# CHECK: fldmiaxeq r7!, {d3-d9}</div><div>+# CHECK: fldmiaxeq r6, {d4-d9}</div><div>+# CHECK: fldmdbx r8!, {d1-d4}</div><div>+# CHECK: fstmiax r3, {d2-d4}</div><div>+# CHECK: fstmiaxeq r3!, {d5-d6}</div><div>+# CHECK: fstmdbxne r0!, {d3-d9}</div><div>Index: utils/TableGen/EDEmitter.cpp</div><div>===================================================================</div><div>--- utils/TableGen/EDEmitter.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ utils/TableGen/EDEmitter.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -700,6 +700,7 @@</div><div> MISC("addr_offset_none", "kOperandTypeARMAddrMode7"); // R</div><div> MISC("reglist", "kOperandTypeARMRegisterList"); // I, R, ...</div><div> MISC("dpr_reglist", "kOperandTypeARMDPRRegisterList"); // I, R, ...</div><div>+ MISC("dpr_reglist_fmx", "kOperandTypeARMDPRRegisterList"); // I, R, ...</div><div> MISC("spr_reglist", "kOperandTypeARMSPRRegisterList"); // I, R, ...</div><div> MISC("it_mask", "kOperandTypeThumbITMask"); // I</div><div> MISC("t2addrmode_reg", "kOperandTypeThumb2AddrModeReg"); // R</div><div>Index: lib/Target/ARM/AsmParser/ARMAsmParser.cpp</div><div>===================================================================</div><div>--- lib/Target/ARM/AsmParser/ARMAsmParser.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ lib/Target/ARM/AsmParser/ARMAsmParser.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -849,6 +849,26 @@</div><div> bool isReg() const { return Kind == k_Register; }</div><div> bool isRegList() const { return Kind == k_RegisterList; }</div><div> bool isDPRRegList() const { return Kind == k_DPRRegisterList; }</div><div>+ bool isDPRRegListFMX() const {</div><div>+ // We need to check that we don't use a D reg with an index greater</div><div>+ // then 15.</div><div>+ if (Kind != k_DPRRegisterList)</div><div>+ return false;</div><div>+ const SmallVectorImpl<unsigned> &list = getRegList();</div><div>+ for (SmallVectorImpl<unsigned>::const_iterator I = list.begin(), </div><div>+ E = list.end();</div><div>+ I != E;</div><div>+ ++I){</div><div>+ // We only allow D registers smaller then 16.</div><div>+ if (!(ARMMCRegisterClasses[ARM::DPRRegClassID].contains(*I))) </div><div>+ return false;</div><div>+ if (getARMRegisterNumbering(*I) > 15) return false; </div></blockquote><div><br></div><div>We have the DPR_VFP2 register class for the low 16 DPR registers. You can just update the .contains() check instead.</div><div><br></div><div>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.</div><br><blockquote type="cite"><div>+ }</div><div>+ // No registers were transfered.</div></blockquote><div><br></div><div>"transferred". That said, I'm not sure what this comment means. No transference is occurring here.</div><br><blockquote type="cite"><div>+ if (list.size() == 0) return false;</div></blockquote><div><br></div><div>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.</div><br><blockquote type="cite"><div>+ </div><div>+ return true;</div><div>+ }</div><div> bool isSPRRegList() const { return Kind == k_SPRRegisterList; }</div><div> bool isToken() const { return Kind == k_Token; }</div><div> bool isMemBarrierOpt() const { return Kind == k_MemBarrierOpt; }</div><div>@@ -1472,6 +1492,16 @@</div><div> addRegListOperands(Inst, N);</div><div> }</div><div> </div><div>+ void addDPRRegListFMXOperands(MCInst &Inst, unsigned N) const {</div><div>+ const SmallVectorImpl<unsigned> &RegList = getRegList();</div><div>+ assert(RegList.size() > 0 && "Register list size must be greater then 0.");</div><div>+</div><div>+ // Add the operand indicating the first register.</div><div>+ Inst.addOperand(MCOperand::CreateReg(RegList[0]));</div><div>+ // Add the immediate operand indicating the number of registers used.</div><div>+ Inst.addOperand(MCOperand::CreateImm(RegList.size()));</div><div>+ }</div><div>+</div></blockquote><div><br></div><div>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.</div><br><blockquote type="cite"><div> void addSPRRegListOperands(MCInst &Inst, unsigned N) const {</div><div> addRegListOperands(Inst, N);</div><div> }</div><div>Index: lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp</div><div>===================================================================</div><div>--- lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -603,6 +603,27 @@</div><div> O << ", asr #" << Imm;</div><div> }</div><div> </div><div>+void ARMInstPrinter::printRegisterFMXList(const MCInst *MI, unsigned OpNum,</div><div>+ raw_ostream &O) {</div><div>+ unsigned BaseReg = MI->getOperand(OpNum).getReg();</div><div>+ unsigned NumRegs = MI->getOperand(OpNum + 1).getImm();</div><div>+</div><div>+ O << "{";</div><div>+ // We will write this with the - format, since the upper registers may not</div><div>+ // exist.</div><div>+ switch(NumRegs){</div><div>+ case 0:</div><div>+ break;</div><div>+ case 1:</div><div>+ O << getRegisterName(BaseReg);</div><div>+ break;</div><div>+ default:</div><div>+ O << getRegisterName(BaseReg);</div><div>+ O << "-d" << getARMRegisterNumbering(BaseReg) + NumRegs - 1;</div></blockquote><blockquote type="cite"><div>+ }</div><div>+ O << "}";</div><div>+}</div><div>+</div></blockquote><div><br></div><div>This can go away and the operand can just reference the existing printRegisterList().</div><br><blockquote type="cite"><div> void ARMInstPrinter::printRegisterList(const MCInst *MI, unsigned OpNum,</div><div> raw_ostream &O) {</div><div> O << "{";</div><div>Index: lib/Target/ARM/InstPrinter/ARMInstPrinter.h</div><div>===================================================================</div><div>--- lib/Target/ARM/InstPrinter/ARMInstPrinter.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ lib/Target/ARM/InstPrinter/ARMInstPrinter.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -114,6 +114,8 @@</div><div> raw_ostream &O);</div><div> void printSBitModifierOperand(const MCInst *MI, unsigned OpNum,</div><div> raw_ostream &O);</div><div>+ void printRegisterFMXList(const MCInst *MI, unsigned OpNum,</div><div>+ raw_ostream &O);</div><div> void printRegisterList(const MCInst *MI, unsigned OpNum, raw_ostream &O);</div><div> void printNoHashImmediate(const MCInst *MI, unsigned OpNum, raw_ostream &O);</div><div> void printPImmediate(const MCInst *MI, unsigned OpNum, raw_ostream &O);</div><div>Index: lib/Target/ARM/ARMInstrVFP.td</div><div>===================================================================</div><div>--- lib/Target/ARM/ARMInstrVFP.td<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ lib/Target/ARM/ARMInstrVFP.td<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -118,7 +118,7 @@</div><div> InstrItinClass itin, InstrItinClass itin_upd> {</div><div> // Double Precision</div><div> def DIA :</div><div>- AXDI4<(outs), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs, variable_ops),</div><div>+ AXDI4_D<(outs), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs, variable_ops),</div><div> IndexModeNone, itin,</div><div> !strconcat(asm, "ia${p}\t$Rn, $regs"), "", []> {</div><div> let Inst{24-23} = 0b01; // Increment After</div><div>@@ -126,7 +126,7 @@</div><div> let Inst{20} = L_bit;</div><div> }</div><div> def DIA_UPD :</div><div>- AXDI4<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs,</div><div>+ AXDI4_D<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs,</div><div> variable_ops),</div><div> IndexModeUpd, itin_upd,</div><div> !strconcat(asm, "ia${p}\t$Rn!, $regs"), "$Rn = $wb", []> {</div><div>@@ -135,7 +135,7 @@</div><div> let Inst{20} = L_bit;</div><div> }</div><div> def DDB_UPD :</div><div>- AXDI4<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs,</div><div>+ AXDI4_D<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist:$regs,</div><div> variable_ops),</div><div> IndexModeUpd, itin_upd,</div><div> !strconcat(asm, "db${p}\t$Rn!, $regs"), "$Rn = $wb", []> {</div><div>@@ -216,7 +216,41 @@</div><div> (VLDMDIA_UPD SP, pred:$p, dpr_reglist:$r)>;</div><div> </div><div> // FLDMX, FSTMX - mixing S/D registers for pre-armv6 cores</div><div>+// These instructions are deprecated, so we don't want them to get selected.</div><div> </div></blockquote><div><br></div><div>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.</div><div><br></div><br><blockquote type="cite"><div>+multiclass vfp_fldst_mult<string asm, bit L_bit,</div><div>+ InstrItinClass itin, InstrItinClass itin_upd> {</div><div>+ // Double Precision</div><div>+ def DIA :</div><div>+ AXDI4_F<(outs), (ins GPR:$Rn, pred:$p, dpr_reglist_fmx:$regs, variable_ops),</div><div>+ IndexModeNone, itin,</div><div>+ !strconcat(asm, "iax${p}\t$Rn, $regs"), "", []> {</div><div>+ let Inst{24-23} = 0b01; // Increment After</div><div>+ let Inst{21} = 0; // No writeback</div><div>+ let Inst{20} = L_bit;</div><div>+ }</div><div>+ def DIA_UPD :</div><div>+ AXDI4_F<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist_fmx:$regs,</div><div>+ variable_ops),</div><div>+ IndexModeUpd, itin_upd,</div><div>+ !strconcat(asm, "iax${p}\t$Rn!, $regs"), "$Rn = $wb", []> {</div><div>+ let Inst{24-23} = 0b01; // Increment After</div><div>+ let Inst{21} = 1; // Writeback</div><div>+ let Inst{20} = L_bit;</div><div>+ }</div><div>+ def DDB_UPD :</div><div>+ AXDI4_F<(outs GPR:$wb), (ins GPR:$Rn, pred:$p, dpr_reglist_fmx:$regs,</div><div>+ variable_ops),</div><div>+ IndexModeUpd, itin_upd,</div><div>+ !strconcat(asm, "dbx${p}\t$Rn!, $regs"), "$Rn = $wb", []> {</div><div>+ let Inst{24-23} = 0b10; // Decrement Before</div><div>+ let Inst{21} = 1; // Writeback</div><div>+ let Inst{20} = L_bit;</div><div>+ }</div><div>+}</div></blockquote><blockquote type="cite"><div>+</div><div>+defm FLDM : vfp_fldst_mult<"fldm", 1, NoItinerary, NoItinerary>;</div><div>+defm FSTM : vfp_fldst_mult<"fstm", 0, NoItinerary, NoItinerary>;</div><div> //===----------------------------------------------------------------------===//</div><div> // FP Binary Operations.</div><div> //</div><div>Index: lib/Target/ARM/ARMInstrInfo.td</div><div>===================================================================</div><div>--- lib/Target/ARM/ARMInstrInfo.td<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ lib/Target/ARM/ARMInstrInfo.td<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -386,6 +386,14 @@</div><div> let DecoderMethod = "DecodeDPRRegListOperand";</div><div> }</div><div> </div><div>+def DPRRegListFMXAsmOperand : AsmOperandClass {let Name = "DPRRegListFMX"; }</div><div>+def dpr_reglist_fmx : Operand<i32> {</div><div>+ let EncoderMethod = "getRegisterFMXListOpValue";</div><div>+ let ParserMatchClass = DPRRegListFMXAsmOperand;</div><div>+ let PrintMethod = "printRegisterFMXList";</div><div>+ let DecoderMethod = "DecodeDPRRegFMXListOperand";</div></blockquote><div><br></div><div>Other than the AsmOperandClass, these can all refer to the same functions as for the normal DPRRegList.</div><br><blockquote type="cite"><div>+}</div><div>+</div><div> def SPRRegListAsmOperand : AsmOperandClass { let Name = "SPRRegList"; }</div><div> def spr_reglist : Operand<i32> {</div><div> let EncoderMethod = "getRegisterListOpValue";</div><div>Index: lib/Target/ARM/ARMCodeEmitter.cpp</div><div>===================================================================</div><div>--- lib/Target/ARM/ARMCodeEmitter.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ lib/Target/ARM/ARMCodeEmitter.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -320,6 +320,8 @@</div><div> unsigned getNEONVcvtImm32OpValue(const MachineInstr &MI, unsigned Op)</div><div> const { return 0; }</div><div> </div><div>+ unsigned getRegisterFMXListOpValue(const MachineInstr &MI, unsigned Op)</div><div>+ const { return 0; }</div><div> unsigned getRegisterListOpValue(const MachineInstr &MI, unsigned Op)</div><div> const { return 0; }</div><div> </div><div>Index: lib/Target/ARM/ARMInstrFormats.td</div><div>===================================================================</div><div>--- lib/Target/ARM/ARMInstrFormats.td<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ lib/Target/ARM/ARMInstrFormats.td<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -1426,16 +1426,32 @@</div><div> </div><div> // Encode instruction operands.</div><div> let Inst{19-16} = Rn;</div><div>- let Inst{22} = regs{12};</div><div> let Inst{15-12} = regs{11-8};</div><div>- let Inst{7-0} = regs{7-0};</div><div>-</div><div>+ let Inst{7-1} = regs{7-1};</div></blockquote><div><br></div><div>Where do the bits being removed here get set for the old instructions from the vfp_ldst_mult multiclass?</div><br><blockquote type="cite"><div> // TODO: Mark the instructions with the appropriate subtarget info.</div><div> let Inst{27-25} = 0b110;</div><div> let Inst{11-9} = 0b101;</div><div> let Inst{8} = 1; // Double precision</div><div>+ </div><div>+ let regs{0} = 0;</div></blockquote><div><br></div><div>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.</div><br><blockquote type="cite"><div> }</div><div> </div><div>+class AXDI4_D<dag oops, dag iops, IndexMode im, InstrItinClass itin,</div><div>+ string asm, string cstr, list<dag> pattern></div><div>+ : AXDI4<oops, iops, im, itin, asm, cstr, pattern> {</div><div>+ let Inst{22} = regs{12};</div><div>+ let Inst{12} = regs{8};</div></blockquote><div><br></div><div>Inst{12} is still set in the base class. Why set it again here?</div><div><br></div><blockquote type="cite"><div>+ let Inst{0} = 0;</div><div>+}</div><div>+</div><div>+class AXDI4_F<dag oops, dag iops, IndexMode im, InstrItinClass itin,</div><div>+ string asm, string cstr, list<dag> pattern></div><div>+ : AXDI4<oops, iops, im, itin, asm, cstr, pattern> {</div><div>+ let Inst{22} = 0;</div><div>+ let Inst{0} = 1;</div><div>+ let regs{12} = 0;</div><div>+}</div><div>+</div><div> class AXSI4<dag oops, dag iops, IndexMode im, InstrItinClass itin,</div><div> string asm, string cstr, list<dag> pattern></div><div> : VFPXI<oops, iops, AddrMode4, 4, im,</div><div>Index: lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp</div><div>===================================================================</div><div>--- lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -283,7 +283,9 @@</div><div> </div><div> unsigned getBitfieldInvertedMaskOpValue(const MCInst &MI, unsigned Op,</div><div> SmallVectorImpl<MCFixup> &Fixups) const;</div><div>-</div><div>+ </div><div>+ unsigned getRegisterFMXListOpValue(const MCInst &MI, unsigned Op,</div><div>+ SmallVectorImpl<MCFixup> &Fixups) const;</div><div> unsigned getRegisterListOpValue(const MCInst &MI, unsigned Op,</div><div> SmallVectorImpl<MCFixup> &Fixups) const;</div><div> unsigned getAddrMode6AddressOpValue(const MCInst &MI, unsigned Op,</div><div>@@ -1335,6 +1337,21 @@</div><div> return lsb | (msb << 5);</div><div> }</div><div> </div><div>+unsigned ARMMCCodeEmitter:: getRegisterFMXListOpValue(const MCInst &MI, </div><div>+ unsigned Op, SmallVectorImpl<MCFixup> &Fixups) const {</div><div>+</div><div>+ unsigned Reg = MI.getOperand(Op).getReg();</div><div>+ unsigned NumRegs = MI.getOperand(Op + 1).getImm();</div><div>+</div><div>+ unsigned RegNo = getARMRegisterNumbering(Reg);</div><div>+ unsigned Binary = 0;</div><div>+</div><div>+ Binary |= (RegNo & 0x1f) << 8;</div><div>+ Binary |= NumRegs * 2;</div><div>+</div><div>+ return Binary;</div><div>+}</div><div>+</div></blockquote><div><br></div><div>This can just use the encoder already present for DPR lists. No need for a new one.</div><br><blockquote type="cite"><div> unsigned ARMMCCodeEmitter::</div><div> getRegisterListOpValue(const MCInst &MI, unsigned Op,</div><div> SmallVectorImpl<MCFixup> &Fixups) const {</div><div>Index: lib/Target/ARM/Disassembler/ARMDisassembler.cpp</div><div>===================================================================</div><div>--- lib/Target/ARM/Disassembler/ARMDisassembler.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 156615)</div><div>+++ lib/Target/ARM/Disassembler/ARMDisassembler.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -197,7 +197,10 @@</div><div> uint64_t Address, const void *Decoder);</div><div> static DecodeStatus DecodeDPRRegListOperand(MCInst &Inst, unsigned Val,</div><div> uint64_t Address, const void *Decoder);</div><div>+static DecodeStatus DecodeDPRRegFMXListOperand(MCInst &Inst, unsigned Val,</div><div>+ uint64_t Address, const void *Decoder);</div><div> </div><div>+</div><div> static DecodeStatus DecodeBitfieldMaskOperand(MCInst &Inst, unsigned Insn,</div><div> uint64_t Address, const void *Decoder);</div><div> static DecodeStatus DecodeCopMemInstruction(MCInst &Inst, unsigned Insn,</div><div>@@ -1237,18 +1240,38 @@</div><div> return S;</div><div> }</div><div> </div><div>+static DecodeStatus DecodeDPRRegFMXListOperand(MCInst &Inst, unsigned Val,</div><div>+ uint64_t Address, const void *Decoder){</div><div>+ DecodeStatus S = MCDisassembler::Success;</div><div>+</div><div>+ unsigned Vd = fieldFromInstruction32(Val, 8, 4);</div><div>+ int regs = fieldFromInstruction32(Val, 1, 7);</div><div>+</div><div>+ if (regs == 0 || Vd + regs > 16)</div><div>+ S = MCDisassembler::SoftFail;</div><div>+</div><div>+ if (!Check(S, DecodeDPRRegisterClass(Inst, Vd, Address, Decoder)))</div><div>+ return MCDisassembler::Fail;</div><div>+</div><div>+ Inst.addOperand(MCOperand::CreateImm(regs));</div><div>+ </div><div>+ return S;</div><div>+}</div><div>+</div></blockquote><div><br></div><div>Likewise.</div><br><blockquote type="cite"><div> static DecodeStatus DecodeDPRRegListOperand(MCInst &Inst, unsigned Val,</div><div> uint64_t Address, const void *Decoder) {</div><div> DecodeStatus S = MCDisassembler::Success;</div><div> </div><div> unsigned Vd = fieldFromInstruction32(Val, 8, 5);</div><div>- unsigned regs = fieldFromInstruction32(Val, 0, 8);</div><div>+ int regs = fieldFromInstruction32(Val, 1, 7);</div><div> </div><div>- regs = regs >> 1;</div><div>+ if (regs == 0 || regs > 16 || Vd+regs > 32) </div><div>+ S = MCDisassembler::SoftFail;</div><div> </div><div> if (!Check(S, DecodeDPRRegisterClass(Inst, Vd, Address, Decoder)))</div><div> return MCDisassembler::Fail;</div><div>- for (unsigned i = 0; i < (regs - 1); ++i) {</div><div>+ </div><div>+ for (int i = 0; i < (regs - 1); ++i) {</div><div> if (!Check(S, DecodeDPRRegisterClass(Inst, ++Vd, Address, Decoder)))</div><div> return MCDisassembler::Fail;</div><div> }</div></blockquote><div><div><br></div></div></div><div><br></div><br><blockquote type="cite"><div lang="EN-GB" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div class="Section1" style="page: Section1; "><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Thanks,<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Silviu<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "> </span></div><div style="border-style: none none none solid; border-left-width: 1.5pt; border-left-color: blue; padding: 0cm 0cm 0cm 4pt; "><div><div style="border-style: solid none none; border-top-width: 1pt; border-top-color: rgb(181, 196, 223); padding: 3pt 0cm 0cm; "><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><b><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, sans-serif; ">From:</span></b><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, sans-serif; "><span class="Apple-converted-space"> </span>Jim Grosbach [mailto:grosbach@<a href="http://apple.com">apple.com</a>]<span class="Apple-converted-space"> </span><br><b>Sent:</b><span class="Apple-converted-space"> </span>22 May 2012 17:32<br><b>To:</b><span class="Apple-converted-space"> </span>Silviu Baranga<br><b>Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><b>Subject:</b><span class="Apple-converted-space"> </span>Re: [PATCH]Add support for fstmx/fldmx instructions on the ARM backend<o:p></o:p></span></div></div></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Hi Silviu,<o:p></o:p></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Do these instructions only support the "IA" suffix? Is that suffix always a required part of the syntax?<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">Index: test/MC/Disassembler/ARM/fstmx-arm.txt<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">===================================================================<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">--- test/MC/Disassembler/ARM/fstmx-arm.txt<span class="apple-tab-span"> <span class="Apple-converted-space"> </span></span>(revision 0)<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+++ test/MC/Disassembler/ARM/fstmx-arm.txt<span class="apple-tab-span"> <span class="Apple-converted-space"> </span></span>(revision 0)<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">@@ -0,0 +1,9 @@<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+# RUN: echo "0x0d 0x0b 0xc7 0x0c" | llvm-mc --disassemble -triple=armv7-linux-gnueabi |& FileCheck %s<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+# RUN: echo "0x0b 0x5b 0xd2 0x0c" | llvm-mc --disassemble -triple=armv7-linux-gnueabi |& FileCheck %s<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+# RUN: echo "0x07 0x1b 0x69 0x0d" | llvm-mc --disassemble -triple=armv7-linux-gnueabi |& FileCheck %s<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+# RUN: echo "0x09 0xeb 0x37 0x0d" | llvm-mc --disassemble -triple=armv7-linux-gnueabi |& FileCheck %s<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+# RUN: echo "0x0d 0xfb 0xa4 0x0c" | llvm-mc --disassemble -triple=armv7-linux-gnueabi |& FileCheck %s<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+# RUN: echo "0x0b 0x3b 0xfb 0x0c" | llvm-mc --disassemble -triple=armv7-linux-gnueabi |& FileCheck %s<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+# CHECK-NOT: vstm<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">+# CHECK-NOT: vldm<o:p></o:p></div></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">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?<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">-Jim<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; ">On May 22, 2012, at 4:05 AM, Silviu Baranga <<a href="mailto:silbar01@arm.com" style="color: purple; text-decoration: underline; ">silbar01@arm.com</a>> wrote:<o:p></o:p></div></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><br><br><o:p></o:p></div><div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">Hi,<o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">The fstmx/fldmx ARM instructions are currently not implemented in the ARM backend.<o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">Although these instructions are deprecated, they should still be supported for assembly and<o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">disassembly purposes.<o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">The patch adds support for these instructions and also fixes a problem with the<o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">vstm/vldm instructions which were colliding with the fstmx/fldmx encoding space.<o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">Please review this patch.<o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">Thanks,<o:p></o:p></span></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">Silviu<o:p></o:p></span></div></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 13.5pt; font-family: Helvetica, sans-serif; "><fstmx.diff><o:p></o:p></span></div></div></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div></div></div><span><fstmx.diff></span></div></blockquote></div><br></div></body></html>