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

[Peng Guo via Phabricator via llvm-commits]

pguo marked 11 inline comments as done.
pguo added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineOperand.h:284
+  /// pass MIR formatter.
+  /// \param PrintDef - whether we want to print `def` on an
+  /// operand which isDef. Sometimes, if the operand is printed before '=', we
----------------
dsanders wrote:
> Capitalization Nitpick: whether -> Whether.
> There's a few other cases of this kind of thing throughout the patch
These are from old commits and I tend not to change them and just follow the convention.


================
Comment at: llvm/include/llvm/CodeGen/MachineOperand.h:285
+  /// \param PrintDef - whether we want to print `def` on an
+  /// operand which isDef. Sometimes, if the operand is printed before '=', we
+  /// don't print `def`. \param IsStandalone - whether we want a verbose output
----------------
dsanders wrote:
> Can we avoid the 'Sometimes' in that sentence? It makes it sound like a bug or a random chance but I think you intended it to mean that 'def' is definitely not printed when that condition is met.
The "sometimes" are from old commits, sorry I made some formatting changes in previous commit and now I revert them.


================
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);
+    }
----------------
arsenm wrote:
> Should this go in the PerTargetMIParsingState?
The parseTargetImmMnemonic will access current Token and call lex(), so IMO it's better to put it in MIParser class.


================
Comment at: llvm/lib/CodeGen/MachineOperand.cpp:484-488
+void MIRFormatter::printIRValue(raw_ostream &OS, const Value &V,
+                                ModuleSlotTracker &MST) {
+  printIRValueReference(OS, V, MST);
+}
+
----------------
arsenm wrote:
> I don't see the point of adding this function? This probably shouldn't be leaking the MIRPrinter into MachineOperand.cpp
The idea is a helper function to print IR value in MIR serialization format, which can be used by target custom pseudo source value.  The target custom pseudo source value can hold IR value reference, it's better to print/parse it in a standard way.


================
Comment at: llvm/lib/CodeGen/MachineOperand.cpp:1172
+      Formatter->printCustomPseudoSourceValue(OS, MST, *PVal);
+      OS << "\"";
       break;
----------------
arsenm wrote:
> Single quotes
Do you mean back quote here?  I intentionally use double quotes here for custom pseudo source value, since back quote `...` is used for quoted IR value, which could be part of the serialization output.  The design is custom pseudo source value is wrapped in double quoted string which will be passed to target specific formatter.  Inside the string it can have back quoted IR value as well as other customized text.


================
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
----------------
arsenm wrote:
> I'm surprised to see these in a quoted form? Is that just the default behavior without implementing something?
Yes.  The quote is used to find the start and end of a custom pseudo source value string representation, and pass the whole string to target specific parser.  The back quote is used for IR const value (QuotedIRValue in MIR lexer), so I choose double quote which will be treated as string constant by MIR lexer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69836