New patch taking Eli's comments into account. Olivier. On Tue, Jul 20, 2010 at 11:09 PM, Eli Friedman <eli.friedman at gmail.com> wrote:> On Tue, Jul 20, 2010 at 1:36 PM, Olivier Meurant > <meurant.olivier at gmail.com> wrote: >>> Seems reasonable, but I haven't looked at the code yet. I would >>> suggest trying to split your work up into separate patches which >>> implement incremental pieces of functionality, then submitting them to >>> llvm-commits for review. That is much easier for us to deal with than >>> large monolithic patches out of tree. >> >> Ok. Sorry for the too big patch. Attached is the first patch adding >> only 2 hooks on TargetMachine and on MCAssembler. Style should be LLVM >> compliant. Apply it with "patch -p0". > > + // Make sure the code model is set. > + setCodeModelForStatic(); > > For the JIT, this should be setCodeModelForJIT; it makes a difference > on, for example, x86-64. I'd also suggest changing the name of the > method appropriately. > > - llvm::OwningPtr<MCObjectWriter> Writer(getBackend().createObjectWriter(OS)); > - if (!Writer) > - report_fatal_error("unable to create object writer!"); > + MCObjectWriter *Writer = Writer_; > + if (Writer == 0) { > + Writer = getBackend().createObjectWriter(OS); > + if (!Writer) > + report_fatal_error("unable to create object writer!"); > + } > > It would be cleaner if you kept around the OwningPtr, but only > assigned to it in the case where the writer take ownership; that way, > you wouldn't have to mess with explicitly deleting the writer. Also, > the surrounding code uses two spaces for indentation, not four. > > I think it looks fine otherwise. > > -Eli >-------------- next part -------------- A non-text attachment was scrubbed... Name: mc_jit_01.patch Type: text/x-patch Size: 4411 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100721/87147d95/attachment.bin>
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...
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>