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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:77
+
+#define DEBUG_TYPE "riscv-compress-instrs"
+#define RISCV_COMPRESS_INSTRS_NAME "RISCV Compress Instructions"
----------------
This is also misleading like the filename.


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:100
+// Return log2(widthInBytes) of load/store done by Opcode.
+static uint8_t log2LdstWidth(unsigned Opcode) {
+  switch (Opcode) {
----------------
There's little reason for this to return uint8_t. Just use unsigned.


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:139
+static int64_t getBaseAdjustForCompression(int64_t Offset, unsigned Opcode) {
+  if (compressibleOffset(Offset, Opcode))
+    return 0;
----------------
Why do we need to `compressibleOffset`? Wouldn't the `Offset & ~compressedLDSTOffsetMask(Opcode)` return 0 for the same cases?


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:146
+// Return true if Reg is in a compressed register class.
+static bool compressedReg(Register Reg) {
+  return RISCV::GPRCRegClass.contains(Reg) ||
----------------
Function name should start with a verb action so `isCompressedReg`


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:153
+// Return true if Opcode is a load for which there exists a compressed version.
+static bool compressibleLoad(const unsigned Opcode) {
+  return Opcode == RISCV::LW || Opcode == RISCV::FLW || Opcode == RISCV::LD ||
----------------
`isCompressibleLoad`


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:159
+// Return true if Opcode is a store for which there exists a compressed version.
+static bool compressibleStore(const unsigned Opcode) {
+  return Opcode == RISCV::SW || Opcode == RISCV::FSW || Opcode == RISCV::SD ||
----------------
`isCompressibleStore`


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:246
+
+    // If RegImm.Reg is modified by this instruction then we cannot optimize
+    // past this instruction. If the register is already compressed then it may
----------------
There should be a comma after `instruction`.


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:247
+    // If RegImm.Reg is modified by this instruction then we cannot optimize
+    // past this instruction. If the register is already compressed then it may
+    // possible to optimize a large offset in the current instruction - this
----------------
Comma after `compressed`


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

https://reviews.llvm.org/D92105