Fix for PR18921.

[dyatkovskiy]

Summary

Please review fix for PR18921.

[dyatkovskiy]

As summary (sorry, I'm still new with it):
Fix for PR18921:
Added GNU Assembler compatiblity definitions for post/pre increments in ARMInstrInfo.td.
Added GNU Assembler compatiblity definitions for thumb2: ARMInstrThumb2.td.
Fixed ARMAsmParser::ParseInstruction GNU compatability branch, so it also works for thumb mode from now.

[t.p.northover]

Hi Stepan,

I don't think the changes to the .td files are needed: basically there's no way for the parser to create a GPRPair operand so the definitions will never trigger.

If I completely remove your changes to .td files, your tests still pass.

If you are convinced, could I ask you to remove the existing LDRD GNU hacks from the .td while you're there?

Cheers.

Tim

[rengolin]

One of the issues in the bug report is that ldrd/strd in Thumb mode accepts the offset up to 1020 (in multiples of 4), and in ARM only 0-255, so not only the encoding is different, but the ranges are different.

I'm assuming this should be supported somewhere else, but would be good to add some of the tests from the bug just to show that they are now accepted.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

5410

can you add tests for both formats?

5419

can you add a negative test for this?

[dyatkovskiy]

I don't think the changes to the .td files are needed: basically there's no way for the parser to create a GPRPair operand so the definitions will never trigger.

Yup, they likely won't. I'll do it then tomorrow.

ldrd/strd in Thumb mode accepts the offset up to 1020 (in multiples of 4)

Is it what test/MC/ARM/ldrd-strd-gnu-arm-bad-imm.s checks? Anyway I'll recheck the reference.

Thanks for review guys! I'll publish fix tomorrow. Its time to sleep now :-)

[dyatkovskiy]

Removed "GNU Assembler extension (compatibility)" definitions from ARMInstrInfo.td and ARMInstrThumb2.td
Updated tests.

[dyatkovskiy]

I see no NL at EOF in test/MC/ARM/ldrd-strd-gnu-arm-bad-imm.s. That will fixed in commit.
Renato,
non gnu ldrd/strd has tests already, so if, in you first inline comment you meant
ldrd r0, r1, [r2, #123]
Then it already tested in arm-ldrd.s, arm-memory-instructions.s, basic-thumb2-instructions.s.

negative tests

Yes, we have ldrd-strd-gnu-thumb-bad-regs.s test for it.

[rengolin]

Thanks! LGTM

[dyatkovskiy]

r205622.
Though it is only first part of PR18921. We also have to deal with vmov.i32 instruction.

[dyatkovskiy]

And thank you for fast reviews!

[dyatkovskiy]

r205622

lib/Target/ARM/ARMInstrInfo.td

                    IIC_iLoad_bh_r, "ldrsb", "\t$Rt, $addr",
                    [(set GPR:$Rt, (sextloadi8 addrmode3:$addr))]>;
 
 let mayLoad = 1, neverHasSideEffects = 1, hasExtraDefRegAllocReq = 1 in {
   // Load doubleword
   def LDRD : AI3ld<0b1101, 0, (outs GPR:$Rt, GPR:$Rt2), (ins addrmode3:$addr),
                    LdMiscFrm, IIC_iLoad_d_r, "ldrd", "\t$Rt, $Rt2, $addr", []>,
              Requires<[IsARM, HasV5TE]>;
-
-  // GNU Assembler extension (compatibility)
-  let isAsmParserOnly = 1 in
-    def LDRD_PAIR : AI3ld<0b1101, 0, (outs GPRPairOp:$Rt), (ins addrmode3:$addr),
-                          LdMiscFrm, IIC_iLoad_d_r, "ldrd", "\t$Rt, $addr", []>,
-                    Requires<[IsARM, HasV5TE]>;
 }
 
 def LDA : AIldracq<0b00, (outs GPR:$Rt), (ins addr_offset_none:$addr),
                     NoItinerary, "lda", "\t$Rt, $addr", []>;
 def LDAB : AIldracq<0b10, (outs GPR:$Rt), (ins addr_offset_none:$addr),
                     NoItinerary, "ldab", "\t$Rt, $addr", []>;
 def LDAH : AIldracq<0b11, (outs GPR:$Rt), (ins addr_offset_none:$addr),
                     NoItinerary, "ldah", "\t$Rt, $addr", []>;

 
 // Store doubleword
 let mayStore = 1, neverHasSideEffects = 1, hasExtraSrcRegAllocReq = 1 in {
   def STRD : AI3str<0b1111, (outs), (ins GPR:$Rt, GPR:$Rt2, addrmode3:$addr),
                     StMiscFrm, IIC_iStore_d_r, "strd", "\t$Rt, $Rt2, $addr", []>,
              Requires<[IsARM, HasV5TE]> {
     let Inst{21} = 0;
   }
-
-  // GNU Assembler extension (compatibility)
-  let isAsmParserOnly = 1 in
-    def STRD_PAIR : AI3str<0b1111, (outs), (ins GPRPairOp:$Rt, addrmode3:$addr),
-                           StMiscFrm, IIC_iStore_d_r, "strd", "\t$Rt, $addr", []>,
-                    Requires<[IsARM, HasV5TE]> {
-      let Inst{21} = 0;
-    }
 }
 
 // Indexed stores
 multiclass AI2_stridx<bit isByte, string opc,
                       InstrItinClass iii, InstrItinClass iir> {
   def _PRE_IMM : AI2ldstidx<0, isByte, 1, (outs GPR:$Rn_wb),
                             (ins GPR:$Rt, addrmode_imm12_pre:$addr), IndexModePre,
                             StFrm, iii,

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

       Operands.insert(Operands.begin() + Idx, ARMOperand::CreateReg(
             NewReg, Op1->getStartLoc(), Op2->getEndLoc()));
       delete Op1;
       delete Op2;
     }
   }
 
   // GNU Assembler extension (compatibility)
-  if ((Mnemonic == "ldrd" || Mnemonic == "strd") && !isThumb() &&
-      Operands.size() == 4) {
-    ARMOperand *Op = static_cast<ARMOperand *>(Operands[2]);
-    assert(Op->isReg() && "expected register argument");
+  if ((Mnemonic == "ldrd" || Mnemonic == "strd")) {
+    ARMOperand *Op2 = static_cast<ARMOperand *>(Operands[2]);
+    ARMOperand *Op3 = static_cast<ARMOperand *>(Operands[3]);
+    if (Op3->isMem()) {
+      assert(Op2->isReg() && "expected register argument");
 
       unsigned SuperReg = MRI->getMatchingSuperReg(
-        Op->getReg(), ARM::gsub_0, &MRI->getRegClass(ARM::GPRPairRegClassID));
+          Op2->getReg(), ARM::gsub_0, &MRI->getRegClass(ARM::GPRPairRegClassID));
 
       assert(SuperReg && "expected register pair");
 
       unsigned PairedReg = MRI->getSubReg(SuperReg, ARM::gsub_1);
 
       Operands.insert(Operands.begin() + 3,
-                    ARMOperand::CreateReg(PairedReg, Op->getStartLoc(),
-                                          Op->getEndLoc()));
+                      ARMOperand::CreateReg(PairedReg,
+                                            Op2->getStartLoc(),
+                                            Op2->getEndLoc()));
+    }
   }
 
   // FIXME: As said above, this is all a pretty gross hack.  This instruction
   // does not fit with other "subs" and tblgen.
   // Adjust operands of B9.3.19 SUBS PC, LR, #imm (Thumb2) system instruction
   // so the Mnemonic is the original name "subs" and delete the predicate
   // operand so it will match the table entry.
   if (isThumbTwo() && Mnemonic == "sub" && Operands.size() == 6 &&

test/MC/ARM/ldrd-strd-gnu-arm-bad-imm.s

+@ RUN: not llvm-mc -triple=armv7-linux-gnueabi %s 2>&1 | FileCheck %s
+.text
+@ CHECK: error: instruction requires: thumb2
+@ CHECK:         ldrd    r0, [r0, #512]
+        ldrd    r0, [r0, #512]
+
+@ CHECK: error: instruction requires: thumb2
+@ CHECK:         strd    r0, [r0, #512]
+        strd    r0, [r0, #512]
 No newline at end of file

test/MC/ARM/ldrd-strd-gnu-arm.s

+@ PR18921
+@ RUN: llvm-mc -triple=armv7-linux-gnueabi -show-encoding < %s | FileCheck %s
+.text
+
+@ CHECK-NOT: .code  16
+
+
+@ CHECK: ldrd  r0, r1, [r10, #32]!     @ encoding: [0xd0,0x02,0xea,0xe1]
+@ CHECK: ldrd  r0, r1, [r10], #32      @ encoding: [0xd0,0x02,0xca,0xe0]
+@ CHECK: ldrd  r0, r1, [r10, #32]      @ encoding: [0xd0,0x02,0xca,0xe1]
+        ldrd    r0, [r10, #32]!
+        ldrd    r0, [r10], #32
+        ldrd    r0, [r10, #32]
+
+@ CHECK: strd  r0, r1, [r10, #32]!     @ encoding: [0xf0,0x02,0xea,0xe1]
+@ CHECK: strd  r0, r1, [r10], #32      @ encoding: [0xf0,0x02,0xca,0xe0]
+@ CHECK: strd  r0, r1, [r10, #32]      @ encoding: [0xf0,0x02,0xca,0xe1]
+        strd    r0, [r10, #32]!
+        strd    r0, [r10], #32
+        strd    r0, [r10, #32]

test/MC/ARM/ldrd-strd-gnu-thumb-bad-regs.s

+@ RUN: not llvm-mc -triple=armv7-linux-gnueabi %s 2>&1 | FileCheck %s
+.text
+.thumb
+@ CHECK: error: invalid operand for instruction
+@ CHECK:         ldrd    r12, [r0, #512]
+        ldrd    r12, [r0, #512]
+
+@ CHECK: error: invalid operand for instruction
+@ CHECK:         strd    r12, [r0, #512]
+        strd    r12, [r0, #512]

test/MC/ARM/ldrd-strd-gnu-thumb.s

+@ PR18921
+@ RUN: llvm-mc -triple=armv7-linux-gnueabi -show-encoding < %s | FileCheck %s
+.text
+.thumb
+
+@ CHECK: .code  16
+
+@ CHECK: ldrd  r0, r1, [r10, #512]!    @ encoding: [0xfa,0xe9,0x80,0x01]
+@ CHECK: ldrd  r0, r1, [r10], #512     @ encoding: [0xfa,0xe8,0x80,0x01]
+@ CHECK: ldrd  r0, r1, [r10, #512]     @ encoding: [0xda,0xe9,0x80,0x01]
+        ldrd    r0, [r10, #512]!
+        ldrd    r0, [r10], #512
+        ldrd    r0, [r10, #512]
+
+@ CHECK: strd  r0, r1, [r10, #512]!    @ encoding: [0xea,0xe9,0x80,0x01]
+@ CHECK: strd  r0, r1, [r10], #512     @ encoding: [0xea,0xe8,0x80,0x01]
+@ CHECK: strd  r0, r1, [r10, #512]     @ encoding: [0xca,0xe9,0x80,0x01]
+        strd    r0, [r10, #512]!
+        strd    r0, [r10], #512
+        strd    r0, [r10, #512]