[PATCH] D130769: [RISCV] Combine and remove redundant ADD/SUB instructions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:42
+class RISCVAddSubCombiner : public MachineFunctionPass {
+  // Instructions that are possible candidates to be combined divided by
+  // destination registers.
----------------
"divided by" -> "keyed by"?


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:147
+        LLVM_DEBUG(dbgs() << "  with instruction:\n    ");
+        LLVM_DEBUG(((MachineInstr *)MIB)->print(dbgs()));
+        LLVM_DEBUG(dbgs() << "\n");
----------------
Why is the cast to MachineInstr* needed? Isn't there an `operator->` on MIB?


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:168
+      auto RegisterUsageIter = ModificationRegisterMonitor.find(MBBI->getOperand(2).getReg());
+      if ( RegisterUsageIter != ModificationRegisterMonitor.end()) {
+        for (const auto &Users : RegisterUsageIter->second) {
----------------
Remove extra space before `RegisterUsageIter`


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:187
+  for (auto &RegistersInfo : ModificationRegisterMonitor) {
+    auto ElementToErase =
+        llvm::find_if(RegistersInfo.second,
----------------
Can this use llvm::erase_if?


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:196
+  }
+  DestinationRegistersToCheck.erase(find(DestinationRegistersToCheck, Reg));
+  CandidatesToBeCombined.erase(Reg);
----------------
llvm::erase_value?


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:259
+
+        if (find(DestinationRegistersToCheck, OperandRegister) !=
+            DestinationRegistersToCheck.end()) {
----------------
llvm::is_contained?


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:261
+            DestinationRegistersToCheck.end()) {
+          if (Operand.isDef() && !Operand.isImplicit() && MBBI->modifiesRegister(OperandRegister)) {
+            // Result of instruction is forbidden, can erase it.
----------------
Why do you need to check `Operand.isDef()` and `modifiedRegister`? Isn't `isDef` enough?


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:243
+  if (TM->getOptLevel() != CodeGenOpt::None)
+    addPass(createRISCVAddSubCombinerPass());
 }
----------------
Should this be done before branch folding since it removes instructions which could reduce jump distance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130769