[PATCH] D92105: [RISCV] Add pre-emit pass to make more instructions compressible

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Drive by review.  I'm mostly using this as an exercise to get familiar with the code and the target, so please don't consider any of my comments blocking.



================
Comment at: llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp:100
+// Return log2(widthInBytes) of load/store done by Opcode.
+static unsigned log2LdstWidth(unsigned Opcode) {
+  switch (Opcode) {
----------------
I would expect us to have a means for querying the width of a load/store somewhere, and for this to simply be the log of an existing utility.  I haven't dug around, but are you sure that doesn't exist?


================
Comment at: llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp:126
+static bool compressibleSPOffset(int64_t Offset, unsigned Opcode) {
+  return log2LdstWidth(Opcode) == 2 ? isShiftedUInt<6, 2>(Offset)
+                                    : isShiftedUInt<6, 3>(Offset);
----------------
Don't we have a means already for computing max offset representable in the addressing of an instruction?  I'd expect us to,  


================
Comment at: llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp:305
+      if (MO.isDef()) {
+        assert(isCompressibleLoad(MI));
+        continue;
----------------
It's not clear me to me why the last instruction has to be a load as this assert would seem to imply.  Could we instead directly assert this is the last instruction somehow?


================
Comment at: llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp:320
+  // This is a size optimization.
+  if (skipFunction(Fn.getFunction()) || !Fn.getFunction().hasMinSize())
+    return false;
----------------
While this is a size opt, I'd expect improving code density to outweigh the effect of an extra register move and/or add.  We should measure, but this might be useful even for O2.  


================
Comment at: llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp:343
+      SmallVector<MachineInstr *, 8> MIs;
+      Register NewReg = analyzeCompressibleUses(MI, RegImm, MIs);
+      if (!NewReg)
----------------
The algorithm here appears to be O(N^2).  Can we do better here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92105