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

[Simon Dardis via llvm-commits]

sdardis added a comment.

Comments inlined. Most of them are small issues, and an omission from the reduction table for LW16, which I think should go into this patch.

There is a second short form load instruction, lwgp. That should be done as a separate patch rather than including in this revision. It will be a small patch anyway.

Thanks.


================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:13
@@ +12,3 @@
+/// TODO: Implement microMIPS64 support.
+/// TODO: Implement support for reducing into lwp/swp instruction.
+/// TODO: Implement support for reducing into lwm/swm instruction.
----------------
Doesn't this patch do this? :)

================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:14
@@ +13,3 @@
+/// TODO: Implement support for reducing into lwp/swp instruction.
+/// TODO: Implement support for reducing into lwm/swm instruction.
+//===----------------------------------------------------------------------===//
----------------
I think we should borrow ARM's load store optimise pass rather than implementing it here.

================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:26-27
@@ +25,4 @@
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/MathExtras.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Target/TargetMachine.h"
----------------
Please avoid unnecessary includes.

================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:30
@@ +29,3 @@
+
+#include <algorithm>
+
----------------
Unnecessary include.

================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:41
@@ +40,3 @@
+/// Order of operands to transfer
+// TODO Will be exteded when additional optimizations are added
+enum OperandTransfer {
----------------
By convention, there should be a colon after 'TODO'. Also, spelling of extended.

================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:151-153
@@ +150,5 @@
+
+  // Attempts to reduce all other Load/Store instructions,
+  // returns true on success
+  static bool ReduceLoadStore(MachineInstr *MI, const ReduceEntry &Entry);
+
----------------
I'm not seeing this function used anywhere. Since there are predicates for stack relative accesses and short form memory accesses, is it required?

================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:172-183
@@ +171,14 @@
+    // ImmField(Shift, LBound, HBound, ImmFieldPosition)
+    {RT_OneInstr, OpCodes(Mips::LW, Mips::LWSP_MM), ReduceLWtoLWSP,
+     OpInfo(OT_OperandsAll), ImmField(2, 0, 32, 2)},
+    {RT_OneInstr, OpCodes(Mips::LW_MM, Mips::LWSP_MM), ReduceLWtoLWSP,
+     OpInfo(OT_OperandsAll), ImmField(2, 0, 32, 2)},
+    {RT_OneInstr, OpCodes(Mips::SW, Mips::SW16_MM), ReduceSWtoSW16,
+     OpInfo(OT_OperandsAll), ImmField(2, 0, 16, 2)},
+    {RT_OneInstr, OpCodes(Mips::SW, Mips::SWSP_MM), ReduceSWtoSWSP,
+     OpInfo(OT_OperandsAll), ImmField(2, 0, 32, 2)},
+    {RT_OneInstr, OpCodes(Mips::SW_MM, Mips::SW16_MM), ReduceSWtoSW16,
+     OpInfo(OT_OperandsAll), ImmField(2, 0, 16, 2)},
+    {RT_OneInstr, OpCodes(Mips::SW_MM, Mips::SWSP_MM), ReduceSWtoSWSP,
+     OpInfo(OT_OperandsAll), ImmField(2, 0, 32, 2)},
+};
----------------
LW16 is missing from this table.

================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:212-213
@@ +211,4 @@
+
+  if (Op >= MI->getNumOperands())
+    return false;
+  if (!MI->getOperand(Op).isImm())
----------------
This should be an assert as calling this function with an out of range Op is an error.

    MI->getOperand(Op) 

in

    if (!MI->getOperand(Op).isImm())

will assert that Op < MI->getNumOperands() anyway. Returning false covers a potential bug.

================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:220-221
@@ +219,4 @@
+
+// Returns true if the variable value has the number of least-significant zero
+// bits equal to shift and if the shifted value is between the bounds
+static bool InRange(int64_t Value, unsigned short Shift, int LBound,
----------------
Capitalise Value and Shift as they refer to arguments of this function.

================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:330-334
@@ +329,7 @@
+
+    if (MI->isBundle()) {
+      continue;
+    }
+    if (MI->isDebugValue())
+      continue;
+
----------------
These two cases can be joined together for clarity.

The second case should use isTransient(). This catches cases where MI is a debug value and other pseudo operations like EHLABEL which do not correspond to physical instruction(s).

Put a comment this check saying something like 'Don't reduce bundled instructions or pseudo operations.' so the intention is obvious.


http://reviews.llvm.org/D15144