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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:2351
+// SP to SP alike
+def : T2Pat<(add          GPRsp:$src, imm1_255_neg:$imm),
+            (t2SUBspImm   GPRsp:$src, imm1_255_neg:$imm)>;
----------------
These patterns are unreachable.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9851
+    if (Inst.getOperand(5).getReg() != 0 || HasWideQualifier)
+      break;
+    unsigned V = Inst.getOperand(2).getImm();
----------------
Is this new behavior?  Or was this handled elsewhere before, somehow?


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:5611
+    if (!Check(S, DecoderGPRRegisterClass(Inst, Rd, Address, Decoder)))
+      return MCDisassembler::Fail;
+  }
----------------
I assume this is supposed to reject `adr sp, #label` etc.  Is this new behavior?  If it is, can you split it into a separate patch? 

Not sure what the `Inst.getNumOperands()` check is supposed to be doing.


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:5620
+      Inst.setOpcode(ARM::t2SUBri12);
+      if (!Check(S, DecoderGPRRegisterClass(Inst, 15, Address, Decoder)))
+        return MCDisassembler::Fail;
----------------
This could probably use a comment.  It looks like it's handling `sub r0, pc, #0`?  (That should actually be a valid instruction, as far as I know.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70680