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

[Craig Blackmore via Phabricator via llvm-commits]

craigblackmore marked 4 inline comments as done.
craigblackmore added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:214
+    bool DefinesTargetReg = false;
+    for (const MachineOperand &MO : MI.operands()) {
+      if (MO.isReg() && MO.getReg() == RegImm.Reg && MO.isDef()) {
----------------
frasercrmck wrote:
> Is this a strict enough check for "definition"? What about super- and sub-registers or clobbers? Would `MI.modifiesRegister` work better (and be simpler)?
Thanks, `MI.modifiesRegister` is better and makes this check much simpler.


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:262
+  // Update registers
+  for (MachineOperand &MO : MI.operands())
+    if (MO.isReg() && MO.getReg() == OldRegImm.Reg) {
----------------
frasercrmck wrote:
> Same question about defined...ness here.
At the moment `isDef` is sufficient as we are only expecting `LW` or `SW`. Given we are post regalloc, that means only physical integer registers (which do not have subregisters). I have added an assert so that it is clear we are only expecting `LW` or `SW` and a comment to say that the definedness check may need to be strengthened if the pass is extended to support more instructions.


================
Comment at: llvm/test/CodeGen/RISCV/compress-instrs.ll:1
+; RUN: llc -mtriple=riscv32 -mattr=+c -filetype=obj < %s \
+; RUN:   | llvm-objdump -d --triple=riscv32 --mattr=+c -M no-aliases - \
----------------
jrtc27 wrote:
> This should be a MIR test running just the new pass so it's less fragile (and can also test more interesting cases where it's hard to coerce CodeGen into creating uncompressible instructions)
Thanks, I have replaced this with a MIR test.


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

https://reviews.llvm.org/D92105