[PATCH] D27508: [CodeGen] Make MachineInstr::isIdenticalTo() symmetric.

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor requested changes to this revision.
andrew.w.kaylor added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/CodeGen/MachineInstr.cpp:1003
   if (isBundle()) {
     // Both instructions are bundles, compare MIs inside the bundle.
     MachineBasicBlock::const_instr_iterator I1 = getIterator();
----------------
The getOpcode() check above should guarantee that Other.isBundle() will return true, but it would be nice to mention that explicitly in the comment here.  As someone reading this code for the first time, I saw one call to isBundle() followed by a comment saying "Both instructions are bundles" and it took me a minute to understand how we could know that.


================
Comment at: lib/CodeGen/MachineInstr.cpp:1005
     MachineBasicBlock::const_instr_iterator I1 = getIterator();
-    MachineBasicBlock::const_instr_iterator E1 = getParent()->instr_end();
+    MachineBasicBlock::const_instr_iterator E1 = getBundleEnd(I1);
     MachineBasicBlock::const_instr_iterator I2 = Other.getIterator();
----------------
These calls to getBundleEnd() are introducing a bit of redundancy.  The implementation of getBundleEnd() iterates from the given instruction until it finds an MI that returns false for isBundledWithSucc().  It should always be a small redundancy, but it could be eliminated by just checking isBundledWithSucc() here.


================
Comment at: lib/CodeGen/MachineInstr.cpp:1011
       ++I2;
-      if (I2 == E2 || !I2->isInsideBundle() || !I1->isIdenticalTo(*I2, Check))
+      if (I1 == E1 || I2 == E2)
+        break;
----------------
I don't really like the way this loop is defined with the actual expected condition for return being checked outside the loop.  You could avoid that if you rewrite it like this:
```
// Loop until we reach the end of both bundles.
while (I1->isBundledWithSucc() && I2->isBundledWithSucc()) {
  // If we've reached the end of just one of the two bundles, but not both,
  // the instructions are not identical.
  if (!I1->isBundledWithSucc() || !I2->isBundledWithSucc())
    return false;
  ...
}

```


https://reviews.llvm.org/D27508