[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:62
+// Check if registers meet CM.MVA01S constraints.
+bool RISCVMoveOpt::isCandidateToMergeMVA01S(DestSourcePair &RegPair) {
+  Register Destination = RegPair.Destination->getReg();
----------------
`const DestSourcePair &RegPair`


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:67
+  // If destination is not a0 or a1.
+  if (Destination == RISCV::X10 || Destination == RISCV::X11)
+    if (RISCV::SR07RegClass.hasSubClassEq(SourceRC))
----------------
Join the two ifs with `&&`


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:68
+  if (Destination == RISCV::X10 || Destination == RISCV::X11)
+    if (RISCV::SR07RegClass.hasSubClassEq(SourceRC))
+      return true;
----------------
`SR07RegClass.contains(Source)` should work I think


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:74
+// Check if registers meet CM.MVSA01 constraints.
+bool RISCVMoveOpt::isCandidateToMergeMVSA01(DestSourcePair &RegPair) {
+  Register Destination = RegPair.Destination->getReg();
----------------
`const DestSourcePair &RegPair`


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:80
+  // If Source is s0 - s7.
+  if (RISCV::SR07RegClass.hasSubClassEq(DestinationRC))
+    if (Source == RISCV::X10 || Source == RISCV::X11)
----------------
SR07RegClass.contains(Dest)


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:80
+  // If Source is s0 - s7.
+  if (RISCV::SR07RegClass.hasSubClassEq(DestinationRC))
+    if (Source == RISCV::X10 || Source == RISCV::X11)
----------------
craig.topper wrote:
> SR07RegClass.contains(Dest)
Join the two ifs with &&


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:134
+  MachineBasicBlock::iterator E = MBBI->getParent()->end();
+  DestSourcePair FirstPair = TII->isCopyInstrImpl(*MBBI).value();
+
----------------
Can you pass the `DestSourcePair` from the caller?


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:173
+
+        if (!ModifiedRegUnits.available(SourceReg) ||
+            !UsedRegUnits.available(SourceReg))
----------------
Why are the available checks different between the two cases? Aren't we always moving the paired copy earlier in the basic block to the location of the first copy. So shouldn't the register checks be the same?


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:192
+// merged into CM.MVA01S or CM.MVSA01.
+bool RISCVMoveOpt::MovOpt(MachineBasicBlock &MBB) {
+  bool Modified = false;
----------------
There is no reason to shorten Move to Mov in this function name. And function names should typically start with a verb. `optimizeBlock` is probably a better name


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:229
+
+  Subtarget = &static_cast<const RISCVSubtarget &>(Fn.getSubtarget());
+  if (!Subtarget->hasStdExtZcmp()) {
----------------
Use `Fn.getSubtarget<RISCVSubtarget>()`


================
Comment at: llvm/lib/Target/RISCV/RISCVMoveOptimizer.cpp:230
+  Subtarget = &static_cast<const RISCVSubtarget &>(Fn.getSubtarget());
+  if (!Subtarget->hasStdExtZcmp()) {
+    return false;
----------------
Drop curly braces


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:351
 void RISCVPassConfig::addPreEmitPass2() {
+  addPass(createRISCVMoveOptimizationPass());
   addPass(createRISCVExpandPseudoPass());
----------------
Should we disable for -O0?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150415