[PATCH] D15144: [mips[microMIPS]] Adding code size reduction pass for MicroMIPS

[Simon Dardis via llvm-commits]

sdardis added a subscriber: sdardis.
sdardis added a reviewer: sdardis.
sdardis added a comment.

I believe this work should be implemented in a similar manner to ARM's codesize reduction passes, Thumb2SizeReduction.cpp and ARMLoadStoreOptimizer.cpp.

Their load store optimizer should be modifiable to work for microMIPS. Reusing their logic should avoid the tricky issue of moving loads and stores past other instructions. I'd suggest dropping all the load/store bundling from this patch and focus on the replacing a instruction with a smaller form.

Some of my comments may overlap with Daniel's as we've both looked this but I've tried to delete any ones that overlapped.


================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:154
@@ +153,3 @@
+  const char *getPassName() const override {
+    return "MicroMips32 instruction size reduction pass";
+  }
----------------
microMIPS is the preferred spelling.

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:399
@@ +398,3 @@
+
+  if (MI->isCall())
+    return true;
----------------
This predicate is too lax. It has to check at least the same candidates as Filler:terminateSearch in MipsDelaySlotFiller.cpp, and also has to check it is not crossing control flow instructions such as wait, pause and branches or instructions such as sync which act as ordering barriers.

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:667-680
@@ +666,16 @@
+
+  if (!found)
+    return false;
+
+  unsigned reg;
+  if (!GetReg(sMI, 0, reg))
+    return false;
+
+  // If some intermediate instruction uses reg,
+  // then reduction is not possible
+  if (RegistersUsed.count(reg))
+    return false;
+
+  NNextMII = std::next(iMII);
+  return ReplaceInstruction(MBB, fMI, sMI, consecutiveForward, Entry);
+}
----------------
All this post loop code should be integrated into the loop body. Rather than 'break'ing out of the loop, in case when you've identified a candidate instruction, I believe you should check the rest of your conditions and if you cannot continue, and immediately return false. If the instruction was an invalid candidate but you can continue the search, update the use set and continue, otherwise you can return ReplaceInstruction(...). Outside the loop body, you should have 'return false'.

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:1
@@ +1,2 @@
+//=== MicroMips32SizeReduction.cpp - MicroMips size reduction pass --------===//
+//
----------------
This file should be called MicroMipsSizeReduction.cpp. This patch is for microMIPS32 but should be sufficiently general that it can be trivially extended to microMIPS64. microMIPS64 support should be a separate patch.

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:8
@@ +7,3 @@
+//
+//===----------------------------------------------------------------------===//
+
----------------
Please include a description of this pass, any relevant deficiencies and restrictions. Such as the fact is does not supprt microMIPS64. That comment should be at the bottom of the description as a TODO:.
It should look like:

```
<Usual LLVM boiler plate.>
//===----------------------------------------------------------------------===//
/// \file
/// This pass is used to reduce the size of instructions where applicable ...
/// ....
/// TODO: implement microMIPS64 support.
//===----------------------------------------------------------------------===//
```

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:28
@@ +27,3 @@
+
+#define DEBUG_TYPE "MicroMips-reduce-size"
+
----------------
"MicroMips-reduce-size" should be "micromips-reduce-size".

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:30
@@ +29,3 @@
+
+STATISTIC(NumReduced, "Number of 32-bit instrs reduced to 16-bit ones");
+STATISTIC(NumTwoOne, "Two instructions reduced to one instruction");
----------------
'instrs' should be 'instructions', no need to abbreviate it.

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:32
@@ +31,3 @@
+STATISTIC(NumTwoOne, "Two instructions reduced to one instruction");
+STATISTIC(NumLwmSwm, "Several lw/sw instr. reduced to one lwm/swm instr.");
+
----------------
Here too.

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:595
@@ +594,3 @@
+
+bool MicroMips32SizeReduce::ReduceMIToLwpSwp(void *v) {
+
----------------
Don't use void * and casts. Instead take a pointer/reference to the relevant type.

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:946
@@ +945,3 @@
+
+  if (bit16) {
+    if (lwm)
----------------
This can be reduced to a unsigned Opcode = <nested ternary operator>; <newline>MIB = BuildMI(...MipsII->get(Opcode));

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:958
@@ +957,3 @@
+
+  for (unsigned int i = 0; i < operands.size(); i++)
+    MIB.addOperand(operands[i]);
----------------
Rather than packing the operands into a vector before picking the opcode, pick the opcode then iterate over instrs structure and add the operands from that directly.

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:971
@@ +970,3 @@
+                                               MachineInstr *fMI,
+                                               MachineInstr *sMI, bool flag,
+                                               const ReduceEntry &Entry) {
----------------
Rename flag to something like 'CopyOperandsForward'.

================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:991
@@ +990,3 @@
+  } else
+    return false;
+
----------------
Check for illegal cases first before building an instruction.

================
Comment at: test/CodeGen/Mips/micromips-lwm-swm-lwp-swp-sw16.ll:3
@@ +2,3 @@
+
+define void @ell_3m_mul_d(double* %m3, double* %m1, double* %m2) #0 {
+entry:
----------------
Can you add CHECK-LABEL: <function name> here to match the function and in all the others?


http://reviews.llvm.org/D15144