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

[Justin Bogner via Phabricator via llvm-commits]

bogner 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;
----------------
Should this be SmallVectorImpl<StringRef> rather than forcing the size? Maybe even an ArrayRef?


================
Comment at: lib/CodeGen/MachineInstr.cpp:1439
   if (!memoperands_empty()) {
-    if (!HaveSemi) {
-      OS << ";";
-      HaveSemi = true;
+    SmallVector<StringRef, 8> SSNs;
+    const LLVMContext *Context = nullptr;
----------------
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.


================
Comment at: lib/CodeGen/MachineInstr.cpp:1446-1447
+      Context = &MF->getFunction().getContext();
+    }
+    if (!Context) {
+      CtxPtr = llvm::make_unique<LLVMContext>();
----------------
Would it be clearer to just use "else"?


================
Comment at: lib/CodeGen/MachineOperand.cpp:469-480
+  case SyncScope::System: {
+    break;
+  }
+  default: {
+    if (SSNs.empty())
+      Context.getSyncScopeNames(SSNs);
+
----------------
I'd probably skip the extraneous curly braces here, since you aren't declaring any variables in these cases.


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


https://reviews.llvm.org/D42377