On Tue, Jun 24, 2014 at 11:01 PM, Alp Toker <[email protected]> wrote: > > On 25/06/2014 08:42, David Blaikie wrote: >> >> On Tue, Jun 24, 2014 at 10:31 PM, Alp Toker <[email protected]> wrote: >>> >>> The attached patch removes code that apparently supported '.bc' binary >>> LLVM >>> bitcode as a frontend input kind. >>> >>> There were no tests and it's unclear if it was in a working state. >> >> That's vaguely surprising. >> >>> I'm >>> comfortable removing this because it doesn't really align with the >>> frontend's primary objective of diagnosing textual input and lowering >>> source >>> code. >> >> It seems about as likely that someone would use this as they would use >> .ll input files. I'm not sure how likely either is. As a developer I >> usually use the llc, etc, tools - but sometimes I pass bitcode to >> clang to link (possibly after using llvm-link to link two bitcode >> files together & I want a linker invocation that has all the standard >> libraries, etc rather than trying to invoke the linker myself or do >> two steps of assemble+link). > > > It feels a little odd to push bitcode binaries through the frontend source > code parsing and diagnostic paths. Perhaps the driver could handle that? > > I figured the use case is to produce clang-style diagnostics that can be > serialized, formatted to support different IDEs and editors etc. and that > would only be relevant to '.ll'. > > No strong inclination either way though if you know better. > > >> >>> The removal lets us parse and diagnose '.ll' IR inputs more efficiently >>> without duplicating file contents into memory. >> >> Curious - how's that the case? > > > ParseAssembly() parses '.ll' source code and uses a LLVM SourceMgr, so has > the same underlying reference counting scheme as clang's SourceManager. That > means we get to not copy the whole file into memory: > >>> - // FIXME: This is stupid, IRReader shouldn't take ownership. >>> - llvm::MemoryBuffer *MainFileCopy = >>> - llvm::MemoryBuffer::getMemBufferCopy(MainFile->getBuffer(), >>> - getCurrentFile()); >>> - >>> llvm::SMDiagnostic Err; >>> - TheModule.reset(ParseIR(MainFileCopy, Err, *VMContext)); >>> + TheModule.reset(ParseAssembly(const_cast<MemoryBuffer *>(MainFile), >>> nullptr, >>> + Err, *VMContext)); > > > ParseIR() takes the LazyIRModule path if the input is binary, and that path > steals ownership which forces us to always copy.
Takes ownership of the MemoryBuffer, is that it? You could just create a shallow MemoryBuffer (MemoryBuffer::getMemBuffer - it doesn't own the underlying memory) and give that to ParseIR - it can own the MemoryBuffer all it wants, that doesn't have to copy all that data. Would that suffice? (and/or presumably we could fix the acutal FIXME and just have that whole API not take ownership, ever?) (CC'ing Lang again - because of the ongoing saga of MemoryBuffer, its weird/conditional/dynamic ownership semantics, etc - something he and I are having ongoing (if rather unactioned) conversations about) > > >> >>> (If there's desire to keep '.bc' support and someone contributes tests >>> for >>> it, I'm fine to explore other ways to achieve this optimisation.) > > > If you want to keep the feature, I can look into spinning the memory saving > another way. I'm not terribly invested in it - just mentioning that I /think/ I've relied on this feature on at least a few occasions but it wouldn't be hard for me to workaround - just one data point. Others may chime in with clearer views. - David > Alp. > > > >>> >>> Requires the recently posted MemoryBuffer patch. >>> >>> Alp. >>> >>> -- >>> http://www.nuanti.com >>> the browser experts >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> > > -- > http://www.nuanti.com > the browser experts > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
