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

Mon Dec 9 13:40:25 PST 2019

efriedma added inline comments.

================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9785
+    Inst.setOpcode(ARM::t2ADDspImm);
+    Inst.addOperand(MCOperand::createReg(0)); // cc_out
+    return true;
----------------
dnsampaio wrote:
> 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?
I was more thinking that the t2ADDri12 and t2ADDspImm12 handling are basically identical.  But I guess add/sub are also almost identical.

================
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);
----------------
dnsampaio wrote:
> efriedma wrote:
> > Please don't copy-paste code.
> Again, not quite sure, but guessing I can reduce the if/else common parts.
Nevermind; I assumed you copy-pasted this without really checking.

Why do we need a C++ DecoderMethod for t2ADDspImm, when we don't need one for t2ADDri?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70680