[PATCH] D69836: [MIR] Target specific MIR formating and parsing

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

Could use some tests for the various parse error conditions



================
Comment at: llvm/include/llvm/CodeGen/MIRFormatter.h:1
+//===-- llvm/CodeGen/MIRFormatter.h ------------------------*- C++ -*-===//
+//
----------------
Should pad this out to be the same length as the other lines


================
Comment at: llvm/include/llvm/CodeGen/MIRFormatter.h:58
+
+  /// Implemente target specific parsing of target custom pseudo source value.
+  virtual bool parseCustomPseudoSourceValue(
----------------
Typo Implemente


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:1504
+
+  // handle the case that mnemonic starts with number
+  if (Token.is(MIToken::IntegerLiteral)) {
----------------
Capitalize and period


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:2536-2540
+  case MIToken::dot: {
+    const auto *TII = MF.getSubtarget().getInstrInfo();
+    if (const auto *Formatter = TII->getMIRFormatter()) {
+      return parseTargetImmMnemonic(OpCode, OpIdx, Dest, *Formatter);
+    }
----------------
Should this go in the PerTargetMIParsingState?


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:3180
+    if (ValueInfo == Slots2Values.end())
+      V = nullptr;
+    else
----------------
Initialize to null at the function entry and remove the = nullptrs throughout the function?


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:3191
+  }
+
+  case MIToken::GlobalValue: {
----------------
Extra blank lines between the switch cases


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:3203
+    auto Source =
+        Token.stringValue().str(); // the source has to be null terminated.
+
----------------
Capitalize


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:3220
+  if (!V)
+    return ErrorCallback(Src.begin(), "unse of undefined IR value");
+
----------------
Typo unse


================
Comment at: llvm/lib/CodeGen/MachineOperand.cpp:484-488
+void MIRFormatter::printIRValue(raw_ostream &OS, const Value &V,
+                                ModuleSlotTracker &MST) {
+  printIRValueReference(OS, V, MST);
+}
+
----------------
I don't see the point of adding this function? This probably shouldn't be leaking the MIRPrinter into MachineOperand.cpp


================
Comment at: llvm/lib/CodeGen/MachineOperand.cpp:1172
+      Formatter->printCustomPseudoSourceValue(OS, MST, *PVal);
+      OS << "\"";
       break;
----------------
Single quotes


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.struct.buffer.store.ll:114
   ; CHECK:   [[V_CMP_EQ_U32_e64_:%[0-9]+]]:sreg_64_xexec = V_CMP_EQ_U32_e64 [[V_READFIRSTLANE_B32_]](s32), [[COPY7]](s32), implicit $exec
-  ; CHECK:   G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.amdgcn.struct.buffer.store), [[COPY4]](s32), [[BUILD_VECTOR]](<4 x s32>), [[COPY5]](s32), [[COPY6]](s32), [[V_READFIRSTLANE_B32_]](s32), 0 :: (dereferenceable store 4 into custom TargetCustom7, align 1, addrspace 4)
+  ; CHECK:   G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.amdgcn.struct.buffer.store), [[COPY4]](s32), [[BUILD_VECTOR]](<4 x s32>), [[COPY5]](s32), [[COPY6]](s32), [[V_READFIRSTLANE_B32_]](s32), 0 :: (dereferenceable store 4 into custom "TargetCustom7", align 1, addrspace 4)
   ; CHECK:   [[S_AND_SAVEEXEC_B64_:%[0-9]+]]:sreg_64_xexec = S_AND_SAVEEXEC_B64 killed [[V_CMP_EQ_U32_e64_]], implicit-def $exec, implicit-def $scc, implicit $exec
----------------
I'm surprised to see these in a quoted form? Is that just the default behavior without implementing something?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69836/new/

https://reviews.llvm.org/D69836