[PATCH] D118663: [AArch64] Adds SUBS and ADDS instructions to the MIPeepholeOpt.

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Nice patch, but I think we might need to be checking the condition that is used too. If we split a single SUBS into two, the second of which is setting the flags, then out of NZCV - N (negative) and Z (zero) will be valid as they produce the same results as before, but C (carry) and V (overflow) might be different than the original, given just the wrong input that does overflow/carry on the first SUB but doesn't on the second SUBS.

So I think all the tests here are valid, because they all use eq or ne conditions, but other conditions might not as be.



================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:381
   MachineFunction *MF = MI.getMF();
-  const TargetRegisterClass *RC =
-      TII->getRegClass(TII->get(Opcode), 0, TRI, *MF);
+  const TargetRegisterClass *RC0 =
+      TII->getRegClass(TII->get(Opcode.first), 0, TRI, *MF);
----------------
Can we give these better register class names? A quick comment looks like it would be useful too, explaining the instruction we are adding and which registers they will use.
```
// NewTmpReg = Opcode.first SrcReg
// NewDstReg = Opcode.second NewTmpReg
```


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:393
+  MRI->constrainRegClass(SrcReg, ORC);
   MRI->constrainRegClass(NewTmpReg, ORC);
   MRI->constrainRegClass(NewDstReg, MRI->getRegClass(DstReg));
----------------
This is technically assuming the two instructions have the same RegClass for Operand 1? I think that will always be the case, so maybe that's OK to keep as-is.


================
Comment at: llvm/test/CodeGen/AArch64/arm64-instruction-mix-remarks.ll:14
 ; YAML:      - INST_b.:     '1'
-; YAML:      - INST_ldr:    '1'
-; YAML:      - INST_movk:   '1'
----------------
Should the ldr still be present? And the orr was already present before? It looks like the test was already missing some input, but we might as well keep the ldr around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118663