[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/ARMAsmPrinter.cpp:1173
       case ARM::t2ADDri:
+      case ARM::t2ADDri12:
+      case ARM::t2ADDspImm:
----------------
I'm a little surprised the ri12 variants were missing here. I guess the frame setup code doesn't use them.  Still fine to add, though.


================
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;
----------------
Do we need to care about the source register here?  t2ADDrr doesn't restrict it.  (I guess that might also be a bug?)


================
Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:1531
+            MI.getOpcode() == ARM::t2ADDri12 ||
+            MI.getOpcode() == ARM::t2ADDspImm ||
+            MI.getOpcode() == ARM::t2ADDspImm12)
----------------
We can't generate t2ADDspImm with a frame index; that wouldn't make any sense.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:2876
+def : T2Pat<(or AddLikeOrOp:$Rn, imm0_4095:$Rm),
+            (t2ADDspImm12 GPRsp:$Rn, imm0_4095:$Rm)>;
 
----------------
This pattern is dead code.  Same for the other new patterns.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:4859
+def : t2InstAlias<"sub${p} $Rdn, $imm",
+           (t2SUBspImm12 GPRsp:$Rdn, GPRsp:$Rdn, imm0_4095:$imm, pred:$p)>;
+
----------------
Need testcases for all these aliases.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:711
+                                                        : ARM::t2SUBri)
+                         : (isThumb1 && Offset < 8 && Base != ARM::SP)
+                               ? ARM::tSUBi3
----------------
Not your patch, but the way Thumb1 is handling SP here doesn't make any sense.  Maybe add a FIXME?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:7728
+      return Error(Operands[4]->getStartLoc(),
+                   "source and destination registers must be sp");
+    break;
----------------
Is this Error actually reachable?  If it is, please add a testcase.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9785
+    Inst.setOpcode(ARM::t2ADDspImm);
+    Inst.addOperand(MCOperand::createReg(0)); // cc_out
+    return true;
----------------
Please don't copy-paste code.


================
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);
----------------
Please don't copy-paste code.


================
Comment at: llvm/test/CodeGen/Thumb2/mve-stacksplot.mir:119
     ; CHECK: frame-setup CFI_INSTRUCTION offset $r4, -36
-    ; CHECK: $sp = frame-setup t2SUBri killed $sp, 1216, 14, $noreg, $noreg
-    ; CHECK: $sp = frame-setup tSUBspi $sp, 1, 14, $noreg
+    ; CHECK: $sp = frame-setup t2SUBspImm12 killed $sp, 1220, 14, $noreg
     ; CHECK: frame-setup CFI_INSTRUCTION def_cfa_offset 1256
----------------
This test should probably be using CHECK-next to make it clear what's happening.  (Please commit separately.)

Is it necessary to change the instruction sequence in this patch?  I'd prefer to split the optimization into a separate patch.


================
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
 
----------------
Why does this need to change?


================
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
----------------
What part of the patch changes the preferred disassembly here? Can it be split into a separate patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70680