[PATCH] D50701: [MI] Change the array of `MachineMemOperand` pointers to be a generically extensible collection of extra info attached to a `MachineInstr`.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/ADT/PointerSumType.h:76-77
+  // address of the zero-tag pointer instead of just the zero-tag pointer
+  // itself. This is especially useful when building `ArrayRef`s out of a single
+  // pointer.
+  union {
----------------
This is... "clever". =P Isn't this going to upset ubsan's active union member sanitizer? Do we need to worry about that?


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:25
 #include "llvm/Analysis/AliasAnalysis.h"
+#include "llvm/CodeGen/MachineMemOperand.h"
 #include "llvm/CodeGen/MachineOperand.h"
----------------
Might not work, but can we forward declare this?


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:31
 #include "llvm/MC/MCInstrDesc.h"
+#include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/ArrayRecycler.h"
----------------
We should be able to forward declare MCSymbol.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1485-1486
+  ///
+  /// This is only valid to call once -- an existing symbol can't safely be
+  /// discarded as anyone could already rely on it.
+  ///
----------------
Perhaps the API should be something like, `getOrCreatePreInstrLabel(const Twine &LabelName = "tmp")`

It's not like we need to insert two labels in front of the same instruction. In fact, it's typically better to avoid that.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1493-1495
+  /// This is only valid to call once -- an existing symbol can't safely be
+  /// discarded as anyone could already rely on it.
+  ///
----------------
ditto


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:5448
   if (UnfoldLoad) {
-    std::pair<MachineInstr::mmo_iterator, MachineInstr::mmo_iterator> MMOs =
-        MF.extractLoadMemRefs(MI.memoperands_begin(), MI.memoperands_end());
-    loadRegFromAddr(MF, Reg, AddrOps, RC, MMOs.first, MMOs.second, NewMIs);
+    auto MMOs = extractLoadMMOs(MI.memoperands(), MF);
+    loadRegFromAddr(MF, Reg, AddrOps, RC, MMOs, NewMIs);
----------------
And, I see you did the thing I commented on in the previous patch. I guess do it in two stages.


Repository:
  rL LLVM

https://reviews.llvm.org/D50701