[PATCH] D12260: CodeGen: Introduce LinkedCodeGen and teach LTOCodeGenerator to use it.

Peter Collingbourne via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 27 14:57:39 PDT 2015


On Tue, Aug 25, 2015 at 06:57:54PM -0700, Duncan P. N. Exon Smith wrote:
> > +        PartFilename += utostr(I);
> 
> I don't love these filenames.  I know it's just for the testing tool,
> but maybe `.o.N` instead of `.oN`?

Done.

> > +++ lib/CodeGen/Parallel.cpp
> 
> Should this be ParallelCodeGen.cpp or ParallelCG.cpp?  I don't have a
> strong opinion myself, but I feel like that vaguely follows the file
> naming conventions better (we tend to name files a little redundantly).

Renamed to ParallelCG.cpp.

> > +#ifdef _MSC_VER
> > +// concrt.h depends on eh.h for __uncaught_exception declaration
> > +// even if we disable exceptions.
> > +#include <eh.h>
> > +#endif
> 
> Can/should this go in an llvm/Support header (that also includes
> `<thread>`) somewhere?

Moved to "include/llvm/Support/thread.h" in r246218.

> (Do we already have this somewhere else?  If so, maybe move it to a
> Support header in a separate commit, and then just reuse it.)

This was being used by lld; I've updated it to use the new header in r246219.

> > +    raw_string_ostream OS(BC);
> 
> Does `raw_svector_ostream` work here?

It does; done.

> > +std::unique_ptr<Module>
> > +LinkedCodeGen(std::unique_ptr<Module> M, ArrayRef<raw_pwrite_stream *> OSs,
> 
> This name doesn't really make sense to me.  Also, we use lowerCamelCase() for
> functions these days.  `splitCodeGen()` maybe?  (You might do better.)

Hmm, I wanted to convey that the outputs of this function are intended to be
linked together, but I can't see a good succinct way to do that. splitCodeGen()
it is then.

Thanks,
-- 
Peter


More information about the llvm-commits mailing list