[PATCH] D150415: [RISCV] Add a pass to merge moving parameter registers instructions for Zcmp

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:31
+  const TargetRegisterInfo *TRI;
+  const RISCVSubtarget *Subtarget;
+
----------------
Subtarget doesn't need to be a member of the class. It's only used in runOnMachineFunction


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:110
+    Sreg1 =
+        ARegInFirstPair == RISCV::X10 ? FirstPair.Source : PairedRegs.Source;
+    Sreg2 =
----------------
Put `ARegInFirstPair == RISCV::X10` in a variable instead of repeating it 4 times.


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:162
+          return I;
+
+      } else if (InstOpcode == RISCV::CM_MVSA01 &&
----------------
Why don't we need to check `UsedRegUnits.available(DestReg)` like we do for CM_MVSA01?


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:168
+          return E;
+
+        if (!ModifiedRegUnits.available(SourceReg))
----------------
Why don't we need to we check ModifiedRegUnits.available(DestReg) like we do for `RISCV::CM_MVA01S`?


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:187
+// merged into CM.MVA01S or CM.MVSA01.
+bool RISCVMoveOpt::MoveOpt(MachineBasicBlock &MBB) {
+  bool Modified = false;
----------------
This function name is just a shortened form of the pass name. It should be a verb phrase as documented here https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150415