[PATCH] D42377: [CodeGen] Use MIR syntax for MachineMemOperand printing

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
Comment at: include/llvm/CodeGen/MachineMemOperand.h:299
+  void print(raw_ostream &OS, ModuleSlotTracker &MST,
+             SmallVector<StringRef, 8> &SSNs, const LLVMContext &Context,
+             const MachineFrameInfo *MFI, const TargetInstrInfo *TII) const;
----------------
bogner wrote:
> Should this be SmallVectorImpl<StringRef> rather than forcing the size? Maybe even an ArrayRef?
`SmallVectorImpl<StringRef>` is better, yes. I wish I could make it an `ArrayRef` but `printSyncScope` is caching the results from `LLVMContext::getSyncScopeNames` in `SSNs`, so we can't use `ArrayRef` here.


================
Comment at: lib/CodeGen/MachineInstr.cpp:1439
   if (!memoperands_empty()) {
-    if (!HaveSemi) {
-      OS << ";";
-      HaveSemi = true;
+    SmallVector<StringRef, 8> SSNs;
+    const LLVMContext *Context = nullptr;
----------------
bogner wrote:
> This is just a dummy vector for the API right? We should be able to make it take less space. Though if we make the argument an ArrayRef we can just pass in None and save the variable entirely.
I made it `<StringRef, 0>`, my guess is that most of the time we will have `SyncScope::System` and won't print anything, but honestly I don't know much about it.


================
Comment at: lib/CodeGen/MachineOperand.cpp:770
     int FrameIndex = getIndex();
-    bool IsFixed = false;
+    bool IsFixed = true;
     StringRef Name;
----------------
bogner wrote:
> Is this an unrelated bug fix that snuck in or something? It isn't obvious to me why it's changing.
My bad, this shouldn't be here.


https://reviews.llvm.org/D42377