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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:181
+      return RegImmPair(RISCV::NoRegister, 0);
+    ;
+
----------------
Stray semicolon


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:189
+    // either of the registers to be compressible and the offset can be a 6 bit
+    // immediate scaled by 4.
+    if (RISCV::SPRegClass.contains(Base)) {
----------------
Can also be scaled by 8 for double-sized quantities (and 16 for us downstream). The scaling isn't really relevant here, just that it has a larger immediate (and no source/destination register restriction, as you already note).


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:194
+    } else {
+      Register Rs2 = MI.getOperand(0).getReg();
+      bool Rs2Compressed = compressedReg(Rs2);
----------------
Not rs2 in the case of loads, so I guess SrcDest or something?


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:202
+      // For loads we can only change the base register since dest is defined
+      // rather than used.
+      if (compressibleStore(Opcode)) {
----------------
I think this would be more helpful if it then went on to say what it actually did for stores


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:205
+        // Cannot resolve uncompressible offset if we are resolving src reg.
+        if (!Rs2Compressed && (BaseCompressed || Rs2 == Base) && !NewBaseAdjust)
+          return RegImmPair(Rs2, NewBaseAdjust);
----------------
How often is storing an address to itself at offset 0 actually hit?..


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:206
+        if (!Rs2Compressed && (BaseCompressed || Rs2 == Base) && !NewBaseAdjust)
+          return RegImmPair(Rs2, NewBaseAdjust);
+      }
----------------
I can't obviously see tests that exercise this?


================
Comment at: llvm/test/CodeGen/RISCV/compress-instrs-i32-f-d.mir:1
+# RUN: llc -o - %s -mtriple=riscv32 -mattr=+c -simplify-mir \
+# RUN:   -run-pass=riscv-compress-instrs | FileCheck %s
----------------
These will need +f and +d given you use F[LS][WD]


================
Comment at: llvm/test/CodeGen/RISCV/compress-instrs-i32-f-d.mir:32
+  entry:
+    store volatile i32 1, i32* null, align 4
+    store volatile i32 3, i32* null, align 4
----------------
Why null for all of these? That seems like a bad idea...


================
Comment at: llvm/test/CodeGen/RISCV/compress-instrs-i32-f-d.mir:80
+  entry:
+    store volatile i32 1, i32* inttoptr (i32 305421696 to i32*), align 4
+    store volatile i32 3, i32* inttoptr (i32 305421700 to i32*), align 4
----------------
This would be better as an offset to a real pointer, not a horrible inttoptr of a constant


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

https://reviews.llvm.org/D92105