[PATCH] D27990: [Thumb] Add support for tMUL in the compare instruction peephole optimizer

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2528
+  // and optimise away the CMP instruction from the generated code sequence:
+  // MULS, MOVS, MOVS, CMP. Here the MOVS instruction load the boolean values
+  // resulting from the select instruction, but these MOVS instructions are
----------------
Typo: MOVS instruction**s** (also on line 2531)


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2537
+    --I;
+    bool CanReorderMOVS = true;
+    const bool HasStmts = I != E;
----------------
Nitpick: I think CanReorder is a better name.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2541
+      if (I->getOpcode() != ARM::tMOVi8 ||
+          I->getOperand(3).getImm() != ARMCC::AL) {
+        CanReorderMOVS = false;
----------------
Why not isPredicated?


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:2541
+      if (I->getOpcode() != ARM::tMOVi8 ||
+          I->getOperand(3).getImm() != ARMCC::AL) {
+        CanReorderMOVS = false;
----------------
rovka wrote:
> Why not isPredicated?
You should cover this in the tests (i.e. check that the optimization is not performed for a predicated MOV).


================
Comment at: test/CodeGen/ARM/cmp1-peephole-thumb.mir:3
+
+# CHECK-NOT: tCMPi8
+
----------------
Nitpick: I'd move this closer to the MIR code. Plus, since the code is dealing with reordering, it's probably a good idea to also check for what you're expecting to see in the output (i.e. the MOVs followed by the MUL).


https://reviews.llvm.org/D27990