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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


================
Comment at: include/llvm/CodeGen/MachineInstr.h:365
   /// \copydoc defs()
   iterator_range<const_mop_iterator> defs() const {
     return make_range(operands_begin(),
----------------
arsenm wrote:
> 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
I agree, will make it a separate commit.

`uses` is also quite confusing, because it's actually `[explicit uses, implicit defs, implicit uses]` range and therefore iterating over it for uses needs to be guarded with additional checks, while all the other ranges are precise and don't need such checks.

Well, "precise" with an assumption that "use" means everything but a register definition, that is register uses, immediates, etc. Which is inconsistent with `MachineOperand::isUse` predicate that asserts `isReg()` along the way.

Looks like in practice the majority of `MachineInstr` clients just gave up and opted out to iterating over all of the operands and doing all the checks they deem necessary. Sadly.


================
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();
 
----------------
arsenm wrote:
> I would probably add a separate getNumDefs for this
Will do.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D45640