[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:1
+//===-- RISCVCompressInstrs.cpp - Make instructions compressible ----------===//
+//
----------------
File name is also misleading. Makes it sound like where instruction compression happens, but it's not, it's a transformation to make the instructions more amenable to compression, at the expense (in most cases) of instruction count.


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:219
+// compressed register is available, return that compressed register.
+static Register analyzeCompressibleUses(MachineBasicBlock &MBB,
+                                        MachineInstr &FirstMI,
----------------
You can get the MBB from the MI, otherwise you risk passing the wrong MBB


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:297
+    if (MO.isReg() && MO.getReg() == OldRegImm.Reg) {
+      // Do not update operands that define the old register.
+      if (MO.isDef()) {
----------------
What if you hit a def of the //new// register?


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:347
+      assert(isInt<12>(RegImm.Imm));
+      BuildMI(MBB, MI, MI.getDebugLoc(), TII.get(RISCV::ADDI), NewReg)
+          .addReg(RegImm.Reg)
----------------
What about when Imm is 0 and all uses can be updated?


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:355
+      for (MachineInstr *UpdateMI : MIs)
+        if (!updateOperands(*UpdateMI, RegImm, NewReg))
+          break;
----------------
Is it ever possible for this to return false on anything other than the final MI? Shouldn't analyzeCompressibleUses have stopped on such an instruction?


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

https://reviews.llvm.org/D92105