[LLVMdev] MC-JIT

[Reid Kleckner]

On Tue, Jul 20, 2010 at 3:41 PM, Olivier Meurant
<meurant.olivier at gmail.com> wrote:
> New patch taking Eli's comments into account.

Comments inline.  If you have commit access, I'd fire away.  If not, I can.

diff --git include/llvm/MC/MCAssembler.h include/llvm/MC/MCAssembler.h
index 07ca070..afff96e 100644
--- include/llvm/MC/MCAssembler.h
+++ include/llvm/MC/MCAssembler.h
@@ -669,7 +669,9 @@ public:
-  void Finish();
+  /// \arg Writer is used for custom object writer (as the MCJIT does),
+  /// if not specified it is automatically created from backend

Nit: End complete sentences end with a period.

+  void Finish(MCObjectWriter *Writer = 0);
diff --git include/llvm/Target/TargetMachine.h
include/llvm/Target/TargetMachine.h
index 227499b..a115877 100644
--- include/llvm/Target/TargetMachine.h
+++ include/llvm/Target/TargetMachine.h
@@ -244,6 +244,15 @@ public:
                                           bool = true) {
     return true;
   }

There wasn't a newline here, but there should be for consistency.  The comment
below should look like the rest of the Doxygen comments:
/// methodName - Use complete sentences starting with caps and ending with
/// periods.

+  /// get machine code emitted.  This method should returns true if fails.
+  /// It fills the MCContext Ctx pointer used to build MCStreamer.
+  ///
+  virtual bool addPassesToEmitMC(PassManagerBase &PM,
+                                 MCContext *&Ctx,
+                                 CodeGenOpt::Level OptLevel,
+                                 bool DisableVerify = true) {
+    return true;
+  }

Ditto.

+  /// get machine code emitted.  This method should returns true if fails.
+  /// It fills the MCContext Ctx pointer used to build MCStreamer.
+  ///
+  virtual bool addPassesToEmitMC(PassManagerBase &PM,
+                                 MCContext *&Ctx,
+                                 CodeGenOpt::Level OptLevel,
+                                 bool DisableVerify = true);

Ditto.

+/// get machine code emitted.  This method should returns true if fails.
+/// It fills the MCContext Ctx pointer used to build MCStreamer.
+///
+bool LLVMTargetMachine::addPassesToEmitMC(PassManagerBase &PM,
+                               MCContext *&Ctx,
+                               CodeGenOpt::Level OptLevel,
+                               bool DisableVerify) {

Args above should be aligned with column after opening paren.

+  // Add common CodeGen passes.
+  if (addCommonCodeGenPasses(PM, OptLevel, DisableVerify, Ctx))
+    return true;
+  // Make sure the code model is set.
+  setCodeModelForJIT();
+
+  return false; // success!
+}
+

-void MCAssembler::Finish() {
+void MCAssembler::Finish(MCObjectWriter *Writer_) {

Why two variables Writer_ and Writer?  I don't know of any rules against
modifying parameters.

...

-  llvm::OwningPtr<MCObjectWriter> Writer(getBackend().createObjectWriter(OS));
-  if (!Writer)
-    report_fatal_error("unable to create object writer!");
+
+  llvm::OwningPtr<MCObjectWriter> OwnWriter(0);
+  MCObjectWriter *Writer = Writer_;
+  if (Writer == 0) {
+    //no custom Writer_ : create the default one life-managed by OwningPtr
+    OwnWriter.reset(getBackend().createObjectWriter(OS));
+    Writer = OwnWriter.get();
+    if (!Writer)
+      report_fatal_error("unable to create object writer!");
+  }

Bit of trailing whitespace on the line above...