[PATCH} Remove '.bc' input support from the frontend

[Eric Christopher]

On Tue, Jun 24, 2014 at 11:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Tue, Jun 24, 2014 at 11:01 PM, Alp Toker <alp at nuanti.com> wrote:
>>
>> On 25/06/2014 08:42, David Blaikie wrote:
>>>
>>> On Tue, Jun 24, 2014 at 10:31 PM, Alp Toker <alp at nuanti.com> 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.
>

I've definitely used the feature before and likely will again. That
said I personally don't mind doing llvm-dis on a bc file before doing
it.

The main reason to keep some aspect of the functionality: It's the
only way to be sure you're actually passing a bit of bitcode through
the precise pipeline that clang has set up. :)

-eric

> - David
>
>> Alp.
>>
>>
>>
>>>>
>>>> Requires the recently posted MemoryBuffer patch.
>>>>
>>>> Alp.
>>>>
>>>> --
>>>> http://www.nuanti.com
>>>> the browser experts
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>
>> --
>> http://www.nuanti.com
>> the browser experts
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits