[PATCH] D50680: [SDAG] Remove the reliance on MI's allocation strategy for `MachineMemOperand` pointers attached to `MachineSDNodes` and instead have the `SelectionDAG` fully manage the memory for this array.

[Justin Bogner via Phabricator via llvm-commits]

bogner accepted this revision.
bogner added a comment.
This revision is now accepted and ready to land.

LGTM as long as Hal's good with your explanation of why we can't use TinyPtrVector.



================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:2258-2262
+  // Note that this could be folded into the above `MemRefs` member if doing so
+  // is advantageous at some point. We don't need to store this in most cases.
+  // However, at the moment this doesn't appear to make the allocation any
+  // smaller and makes the code somewhat simpler to read.
+  int NumMemRefs = 0;
----------------
Yep, sizeof(MachineSDNode) is 96 bytes either way (it's exactly 96 before this change, and 92 + 4 bytes padding after). I don't know if we need quite this long of a comment explaining that it's fine, but I guess it doesn't hurt.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:2270
-  /// list. This does not transfer ownership.
-  void setMemRefs(mmo_iterator NewMemRefs, mmo_iterator NewMemRefsEnd) {
-    for (mmo_iterator MMI = NewMemRefs, MME = NewMemRefsEnd; MMI != MME; ++MMI)
----------------
Should the doxygen of MachineSDNode mention SelectionDAG::setNodeMemRefs somewhere?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7112
+  N->MemRefs = MemRefsBuffer;
+  N->NumMemRefs = (int)NewMemRefs.size();
+}
----------------
C++ style casts are arguably a bit clearer (but feel free to ignore this if you disagree)


Repository:
  rL LLVM

https://reviews.llvm.org/D50680