[PATCH] D70680: [ARM][Thumb2] Fix ADD/SUB invalid writes to SP

[Diogo N. Sampaio via Phabricator via llvm-commits]

dnsampaio marked 14 inline comments as done.
dnsampaio added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:3285
       // we can handle a larger range of immediates.
+      const bool ToSP = DefMI.getOperand(0).getReg() == ARM::SP;
+      const unsigned t2ADD = ToSP ? ARM::t2ADDspImm : ARM::t2ADDri;
----------------
efriedma wrote:
> Do we need to care about the source register here?  t2ADDrr doesn't restrict it.  (I guess that might also be a bug?)
>From my tests, `rr` does block writing to `SP` and not reading from it:
```./llvm-mc --assemble  -triple=thumbv7-apple-darwin9 -mcpu=cortex-a9 --show-encoding <<< "add.w sp, r0, r1"
        .section        __TEXT,__text,regular,pure_instructions
<stdin>:1:7: error: source register must be sp if destination is sp
add.w sp, r0, r1```

In that case, just checking that the destination is `SP` is enough.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:925-938
+    bits<4>  Rn = 13;
+    bits<4>  Rd = 13;
+    bits<12> imm;
+
+    let Inst{31-27} = 0b11110;
+    let Inst{26}    = imm{11};
+    let Inst{25-24} = 0b01;
----------------
john.brawn wrote:
> This duplicates a lot of what's in T2sTwoRegImm, and also incorrectly forces bit 20 (which encodes the 's' bit) to 0 causing adds to be encoded as add.
> 
> I think what should be here is the same as the ri variant, but with "let Rn = 13; let Rd = 13;" at the top.
Is there a special way to do those let? From the ones I managed to compile, using
```
let Rn = 13 in
let Rd = 13 in
def spImm...```
I kept getting ` error: Duplicate predicate in FastISel table!`. So I reduced as much as possible, fixing not setting the `S` bit (20).


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9785
+    Inst.setOpcode(ARM::t2ADDspImm);
+    Inst.addOperand(MCOperand::createReg(0)); // cc_out
+    return true;
----------------
efriedma wrote:
> Please don't copy-paste code.
I'm guessing you are speaking about the ADD and SUB joining in a single case statement, right?


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:6630
+static DecodeStatus DecodeT2AddSubSPImm(MCInst &Inst, unsigned Insn,
+                                        uint64_t Address, const void *Decoder) {
+  const unsigned Rd = fieldFromInstruction(Insn, 8, 4);
----------------
efriedma wrote:
> Please don't copy-paste code.
Again, not quite sure, but guessing I can reduce the if/else common parts.


================
Comment at: llvm/test/MC/Disassembler/ARM/thumb-tests.txt:285
 # CHECK: and.w	r5, r1, r10, ror #7
-0x1 0xea 0xfa 0x95
+0x01,0xea,0xfa,0x15
 
----------------
efriedma wrote:
> Why does this need to change?
That was just a mistake in submitting this patch, wanted to investigate why this test keeps warning, as this (and printing a instruction of different encoding):
```llvm-mc --disassemble --show-encoding -triple=thumbv7-apple-darwin9 -mcpu=cortex-a9 <<< "0x1 0xea 0xfa 0x95"
<stdin>:1:1: warning: potentially undefined instruction encoding
0x1 0xea 0xfa 0x95
^
        and.w   r5, r1, r10, ror #7     @ encoding: [0x01,0xea,0xfa,0x15]
```



================
Comment at: llvm/test/MC/Disassembler/ARM/thumb2.txt:94
+# CHECK: adr.w r11, #-3270
+# CHECK: adr.w r11, #-826
 # CHECK: subw r1, pc, #0
----------------
efriedma wrote:
> What part of the patch changes the preferred disassembly here? Can it be split into a separate patch?
These broke as soon as I've changed the table-gen, will try to pin-point what change it and see if can be done into other patch. Most unlikely, as the adr disassembly part was never used.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70680/new/

https://reviews.llvm.org/D70680