[LLVMdev] MC-JIT

[Olivier Meurant]

New patch. Thanks for all of your comments !

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

I don't have commit access, if you find it ok, please commit it. :)

Olivier.


On Wed, Jul 21, 2010 at 6:56 AM, Reid Kleckner <reid.kleckner at gmail.com> wrote:
> 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...
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mc_jit_01.patch
Type: text/x-patch
Size: 4791 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100721/fc9210df/attachment.bin>