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

[Craig Blackmore via Phabricator via llvm-commits]

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


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


================
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()) {
----------------
jrtc27 wrote:
> What if you hit a def of the //new// register?
I have added a comment about this.


```
      // The new register was scavenged for the range of instructions that are
      // being updated, therefore it should not be defined within this range
      // except possibly in the final instruction.
```


================
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)
----------------
jrtc27 wrote:
> What about when Imm is 0 and all uses can be updated?
I have added a TODO to possibly update all uses in the future.


================
Comment at: llvm/lib/Target/RISCV/RISCVCompressInstrs.cpp:355
+      for (MachineInstr *UpdateMI : MIs)
+        if (!updateOperands(*UpdateMI, RegImm, NewReg))
+          break;
----------------
jrtc27 wrote:
> 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?
I have removed the check of the return value and removed the bool from updateOperands since it is no longer used.


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

https://reviews.llvm.org/D92105