[PATCH] D15913: Consolidate MemRefs handling from BranchFolding and correct latent bug

[Geoff Berry via llvm-commits]

gberry added a subscriber: gberry.
gberry added a comment.

This seems reasonable to me.  A couple of minor comments/questions below.


================
Comment at: lib/CodeGen/BranchFolding.cpp:796
@@ -795,3 +784,1 @@
-      if (!hasIdenticalMMOs(&*MBBI, &*MBBICommon))
-        MBBICommon->dropMemRefs();
 
----------------
I think dropMemRefs() is dead after this change, so it could also be removed.

================
Comment at: lib/CodeGen/MachineInstr.cpp:881
@@ +880,3 @@
+  if (NumMemRefs == Other.NumMemRefs &&
+      std::equal(memoperands_begin(), memoperands_end(),
+                 Other.memoperands_begin()))
----------------
Isn't this going to do pointer equality comparisons instead of the MachineMemOperand::operator==() comparisons that hasIdenticalMMOs() was doing?  I don't think these are equivalent since as far as I can tell there is no uniqueing of MachineMemOperands done at creation.

================
Comment at: lib/CodeGen/MachineInstr.cpp:887
@@ -874,3 +886,3 @@
   // space usage and fall back to conservative information less often.
   size_t CombinedNumMemRefs = (memoperands_end() - memoperands_begin())
     + (Other.memoperands_end() - Other.memoperands_begin());
----------------
Perhaps NumMemRefs + Other.NumMemRefs would be better?


http://reviews.llvm.org/D15913