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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

I don't understand why this is still limited to just LW and SW; you have all the bits for dealing with the differing immediate shifts already so it should be ~no effort to add LD and SD on top, and floating-point variants are just supporting additional register classes for the source and destination, some of which you again already have code for.



================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:108
+  case RISCV::FSW:
+    return 32;
+  case RISCV::LD:
----------------
Comment says bytes, this is bits


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:124
+  case 32:
+    return 0b1111100;
+  case 64:
----------------
This is just 31 * width_in_bytes (or 31 << log2(width_in_bytes))


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:133
+static bool compressibleSPOffset(int64_t Offset, unsigned Opcode) {
+  switch (ldstWidth(Opcode)) {
+  default:
----------------
If this returned log2(bytes) (or you could compute it, but every user of the function is happy with log2(bytes)) then this is just
```
return isUIntN(6 + log2(bytes), Offset) && (Offset % (UINT64_C(1) << log2(bytes)) == 0);
```
(inlining a non-templated version of isShiftedUInt, since we don't seem to have an isShiftedUIntN in MathExtras.h, though perhaps it'd be better to just add it). Ditto for compressibleOffset just with 5 not 6.


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:163
+  // Return the excess bits that do not fit in a compressible offset.
+  return Offset & ~(compressedLDSTOffsetMask(Opcode));
+}
----------------
You don't need the parens


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:214
+  case RISCV::SW: {
+    const MachineOperand &MOImm = MI.getOperand(2);
+    if (!MOImm.isImm())
----------------
There's a lot of duplicated code between loads and stores


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

https://reviews.llvm.org/D92105