[PATCH] Machine Level IR Serialization: print and parse LLVM IR.

[Matthias Braun]

This is mostly a skeleton of the API, so I can't say much about the algorithms/implementation. Also this is one of your first commits so this will be a lot of nitpicking:

In general: For what the code does at the moment there are too many .cpp and .h files for my taste, better start simple and self-contained and break it up later when it has grown (the breaking apart will happen naturally, merging stuff together usually never happens).


REPOSITORY
  rL LLVM

================
Comment at: include/llvm/CodeGen/MIR/Parser.h:30-31
@@ +29,4 @@
+
+/// \brief This function is the main interface to the MIR serialization format
+/// parser.
+/// It reads a YAML file that has an optional LLVM IR and returns an LLVM
----------------
I changed the doxygen settings yesterday so you don't need to prepend \brief anymore if you have exactly one sentence as brief description.

================
Comment at: include/llvm/InitializePasses.h:291
@@ -290,2 +290,3 @@
 void initializeMachineFunctionPrinterPassPass(PassRegistry&);
+void initializeMIRPrintingPassPass(PassRegistry &);
 void initializeStackMapLivenessPass(PassRegistry&);
----------------
It may not be llvm coding style but all the others declarations below and above have no extra space in fron of the & here...

================
Comment at: include/llvm/Support/YAMLTraits.h:1093-1094
@@ -1092,1 +1092,4 @@
 
+  /// \brief Returns the current node that's being parsed by the YAML Parser.
+  const Node *getCurrentNode();
+
----------------
This does not modify the object and should be marked const.

================
Comment at: lib/CodeGen/MIR/MIRParser.h:29-31
@@ +28,5 @@
+
+/// \brief This class implements the parsing of LLVM IR that's embedded inside
+/// of a MIR file.
+class MIRParserImpl {
+  SourceMgr SM;
----------------
My understanding is that this class is only used internally to implement the parser and is not intended for external users. It may be a good idea to move it to the MIRParser.cpp file then and not provide a header for it to make this fact obvious.

================
Comment at: lib/CodeGen/MIR/MIRPrintingPasses.cpp:1
@@ +1,2 @@
+//===- MIRPrintingPasses.cpp - Passes that print out using the MIR format -===//
+//
----------------
Call this MIRPrintingPass.cpp unless you intend to really implement multiple different printing passes.

================
Comment at: lib/CodeGen/MIR/MIRPrintingPasses.cpp:26-27
@@ +25,4 @@
+
+/// MIRPrintingPass - This pass prints out the LLVM IR to an output stream using
+/// the MIR serialization format.
+struct MIRPrintingPass : public ModulePass {
----------------
Don't repeat the type name in the documentation comment.

================
Comment at: lib/CodeGen/MIR/Parser.cpp:22-23
@@ +21,4 @@
+                                           LLVMContext &Context) {
+  auto FileOrErr = MemoryBuffer::getFile(Filename);
+  if (auto EC = FileOrErr.getError()) {
+    Error = SMDiagnostic(Filename, SourceMgr::DK_Error,
----------------
Generally the auto keyword is very unfriendly towards readers of your code. On the other hand having very longish types isn't friendly either. So I'm find with avoiding the Error<std::unique_ptr<...>> monster in the getFile() - although the best solution here would be a typedef in the MemoryBuffer class that can be used instead, but that should be part of a separate patch. The 2nd auto should definitely be replaced by std::error.

================
Comment at: lib/CodeGen/MIR/YAMLMapping.cpp:1
@@ +1,2 @@
+//===- YAMLMapping.cpp - Describes the mapping between MIR and YAML -------===//
+//
----------------
Call it MIRPrinter.cpp like the declarations inside.

================
Comment at: lib/CodeGen/MIR/YAMLMapping.cpp:32
@@ +31,3 @@
+  static StringRef input(StringRef Str, void *Ctxt, Module &Mod) {
+    assert(false && "LLVM Module is supposed to be parsed separately");
+    return "";
----------------
Use llvm_unreachable instead of assert(false);

================
Comment at: lib/CodeGen/MIR/YAMLMapping.h:1
@@ +1,2 @@
+//===- YAMLMapping.h - Describes the mapping between MIR and YAML ---------===//
+//
----------------
Call it MIRPrinter.h like the class that is declared within.

================
Comment at: lib/CodeGen/MIR/YAMLMapping.h:24-33
@@ +23,12 @@
+
+/// \brief This class prints out the LLVM IR using the MIR serialization format
+/// and YAML I/O.
+class MIRPrinter {
+  raw_ostream &OS;
+
+public:
+  MIRPrinter(raw_ostream &OS);
+
+  void printModule(Module &Mod);
+};
+
----------------
This looks to me like the external interface can be reduced to something like "void printMIR(raw_ostream &OS, const Module &Mod);" and leave the class internal to the .cpp file.

http://reviews.llvm.org/D9616

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/