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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

Thanks for doing this. It should make a lot of things easier to read.

In addition to Matt's comments, I have a few other minor comments.



================
Comment at: llvm/include/llvm/CodeGen/MIRFormatter.h:36
+  /// value of OpIdx is ~0 which means the index is unknown.
+  virtual void printImm(raw_ostream &OS, const MachineInstr &MI, unsigned OpIdx,
+                        int64_t Imm) const {
----------------
This is just an optional nitpick. Using ~0 is fine but if you used Optional<unsigned> then you wouldn't have to explain that. It does take a little more storage though so I don't mind if you stick to ~0


================
Comment at: llvm/include/llvm/CodeGen/MIRFormatter.h:73
+
+  /// Helper functions to parse IR value from MMIR serialization format which
+  /// will be useful for target specific parser, e.g. for parsing IR value for
----------------
Typo? MMIR


================
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
----------------
Capitalization Nitpick: whether -> Whether.
There's a few other cases of this kind of thing throughout the patch


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69836