[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/CMakeLists.txt:26
   RISCVMakeCompressible.cpp
+  RISCVAddSubCombiner.cpp
   RISCVExpandAtomicPseudoInsts.cpp
----------------
This list is supposed to be alphabetical. Looks like MakeCompressible broke that, but we shouldn't make it worse.


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:115
+    return false;
+  Register BaseRegister = MBBI->getOperand(0).getReg();
+  if (MachineInstr *CheckedInstr = CandidatesToBeCombined[BaseRegister]) {
----------------
Move this above the earlier if and use `BaseRegister` in the if


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:171
+      assert(CheckedInstr->getOpcode() != MBBI->getOpcode());
+      // Cann't combine if the last operand was modified between current
+      // instructions.
----------------
Cann't -> Can't


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:176
+        for (const auto &Users :
+             ModificationRegisterMonitor[MBBI->getOperand(2).getReg()]) {
+          if (Users.first == BaseRegister && Users.second) {
----------------
Reuse the iterator returned from find above


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:192
+
+void RISCVAddSubCombiner::excludeFromCandidatesByRegister(const Register &Reg) {
+  for (auto &RegistersInfo : ModificationRegisterMonitor) {
----------------
Register is wrapper around unsigned. Pass it by value not reference


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:195
+    auto ElementToErase =
+        std::find_if(RegistersInfo.second.begin(), RegistersInfo.second.end(),
+                     [&Reg](std::pair<Register, bool> Element) {
----------------
llvm::find_if


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:236
+  ModificationRegisterMonitor.clear();
+  bool Modified = false;
+
----------------
It doesn't look like Modified is assigned anywhere else in this function


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:256
+         OperandIndex--) {
+      auto Operand = MBBI->getOperand(OperandIndex);
+
----------------
This should be a reference.


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:263
+            MBBI->modifiesRegister(OperandRegister)) {
+          for (auto &Users : ModificationRegisterMonitor[OperandRegister]) {
+            Users.second = true;
----------------
Drop curly braces


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:270
+            DestinationRegistersToCheck.end()) {
+          if (OperandIndex == 0 && MBBI->modifiesRegister(OperandRegister)) {
+            // Result of instruction is forbidden, can erase it.
----------------
Do we have to assume there is only 1 def? Can we check isDef on the operand instead of checking 0?


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:293
+      }
+      if (find(IgnoredFrameRegisters, MBBI->getOperand(0).getReg()) !=
+          IgnoredFrameRegisters.end()) {
----------------
Can this be llvm::is_contained?


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:305
+      if (MBBI->getOperand(2).isReg()) {
+        if (ModificationRegisterMonitor.find(MBBI->getOperand(2).getReg()) ==
+            ModificationRegisterMonitor.end()) {
----------------
You don't need to manually construct an empty vector here. Calling operator[] on line 310 will default construct the vector if it doesn't already exist. Then the push_back will be called.


================
Comment at: llvm/lib/Target/RISCV/RISCVAddSubCombiner.cpp:323
+  Subtarget = &MF.getSubtarget<RISCVSubtarget>();
+  TII = static_cast<const RISCVInstrInfo *>(Subtarget->getInstrInfo());
+
----------------
RISCVSubtarget::getInstrINfo returns a RISCVInstrInfo *. No need for static_cast


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:242
   addPass(createRISCVMakeCompressibleOptPass());
+  if (TM->getOptLevel() > CodeGenOpt::None)
+    addPass(createRISCVAddSubCombinerPass());
----------------
Use != like the other places in this file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130769