[PATCH] D14797: [BranchFolding] Merge MMOs during tail merge

[Geoff Berry via llvm-commits]

gberry added inline comments.

================
Comment at: lib/CodeGen/BranchFolding.cpp:747
@@ -746,2 +746,3 @@
 
-static bool hasIdenticalMMOs(const MachineInstr *MI1, const MachineInstr *MI2) {
+// Add MI1's MMOs to MI2's.
+static void mergeMMOs(MachineInstr *MI1, MachineInstr *MI2) {
----------------
Maybe you should mention the fact that you are removing duplicates.

================
Comment at: lib/CodeGen/BranchFolding.cpp:751
@@ -749,6 +750,3 @@
   auto I2 = MI2->memoperands_begin(), E2 = MI2->memoperands_end();
-  if ((E1 - I1) != (E2 - I2))
-    return false;
-  for (; I1 != E1; ++I1, ++I2) {
-    if (**I1 != **I2)
-      return false;
+  if ((E1 - I1) == 0)
+    return;
----------------
This check seems unnecessary to me.  You will just skip the below for loop if this is true.

================
Comment at: lib/CodeGen/BranchFolding.cpp:754
@@ +753,3 @@
+
+  for (; I1 != E1; ++I1) {
+    bool IsDupMMO = false;
----------------
You should consider using a SmallSet here to look for duplicates instead of the N^2 search that you are currently doing.

================
Comment at: lib/CodeGen/BranchFolding.cpp:756
@@ +755,3 @@
+    bool IsDupMMO = false;
+    for (; I2 != E2; ++I2) {
+      if (**I1 == **I2) {
----------------
I think you need to set I2 = MI2->memoperands_begin() and E2 = MI2->memoperands_end() here.  Otherwise, whenever you find an MMO is equal, when you start looking for a matching MMO on the next iteration you will start where you left off, instead of at the beginning of MI2's MMO list.

================
Comment at: lib/CodeGen/BranchFolding.cpp:763
@@ +762,3 @@
+    if (IsDupMMO == false) {
+      MachineFunction *MF = MI1->getParent()->getParent();
+      MI2->addMemOperand(*MF, *I1);
----------------
This can be hoisted out of the loop.


http://reviews.llvm.org/D14797