[PATCH] D45640: [MIR][MachineCSE] Implementing proper MachineInstr::getNumExplicitDefs()

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: wdng.

LGTM



================
Comment at: include/llvm/CodeGen/MachineInstr.h:365
   /// \copydoc defs()
   iterator_range<const_mop_iterator> defs() const {
     return make_range(operands_begin(),
----------------
Somewhat related, I think these need to be renamed to explicit_defs(). I've fixed similar bugs before from not checking the implicit def physical registers


================
Comment at: lib/CodeGen/MachineCSE.cpp:555-556
     bool DoCSE = true;
-    unsigned NumDefs = MI->getDesc().getNumDefs() +
-                       MI->getDesc().getNumImplicitDefs();
+    unsigned NumDefs =
+        MI->getNumExplicitDefs() + MI->getDesc().getNumImplicitDefs();
 
----------------
I would probably add a separate getNumDefs for this


================
Comment at: lib/CodeGen/MachineInstr.cpp:528-529
+    const MachineOperand &MO = getOperand(I);
+    if (MO.isReg() && MO.isImplicit())
+      break;
+    ++NumOperands;
----------------
Maybe needs a comment explaining implicit operands always follow explicit


Repository:
  rL LLVM

https://reviews.llvm.org/D45640