[PATCH] Adding a timing option for IR parsing

[Chandler Carruth]

On Thu, Mar 21, 2013 at 8:30 PM, Andrew Trick <atrick at apple.com> wrote:

>
> On Mar 21, 2013, at 8:23 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
> On Thu, Mar 21, 2013 at 8:20 PM, Andrew Trick <atrick at apple.com> wrote:
>
>>
>> On Mar 21, 2013, at 7:24 PM, Chandler Carruth <chandlerc at google.com>
>> wrote:
>>
>> FYI, I reverted the patch in r177695 to fix our builds. We check that
>> layering violations don't get any worse in our build system.
>>
>> Fundamentally, the existing layering problem is "worked around" but there
>> being no implementation. As long as thats true, all the Support library is
>> providing is a text header file. As long as it is only included into source
>> files that are part of a library which will eventually get linked with the
>> core library, the AsmParser library, and the BitcodeReader library, all is
>> well. Including it into a source file in Support makes the layering
>> violation actively worse.
>>
>> I think the correct solution is to do one of two things:
>>
>> 1) Create a new library which depends on both AsmParser and BitcodeReader
>> and which can be used by opt and friends. Put IRReader here.
>> 2) Fold AsmParser into the IR library (where AsmWriter already lives).
>> Put IRReader into the BitcodeReader library.
>>
>> I thought about a third option: put both AsmParser and IRReader into the
>> BitcodeReader library. But increasingly that seems like a really gross
>> layering.
>>
>>
>> I like option #2, but some people might not want to link in the largish
>> parser lib.
>>
>
> I find this surprising given the relative size of the IR library... But
> I've heard the same before. Maybe someone else can provide some clarity on
> exactly what the concern is here.
>
>
>>
>> Seems like the IR library should provide a text header IRReader.h. Anyone
>> calling it should also link AsmParser and BitcodeReader.  I don't think we
>> need IRReader.cpp, but if we did it would be implemented in /IR and linked
>> in LLVMCore.
>>
>
> If IRReader depends on BitcodeReader, we cannot put it in the IR tree, or
> link in its implementation in LLVMCore. That's the the same kind of
> layering violation we already have.
>
>
> Nothing would refer to BitcodeReader symbols except the inline function.
> Is there any real problem, or just conceptual?
>

... is the conceptual problem not enough?

Historically, LLVM has a very long and painful history of putting inline
functions in a header file that violate the conceptual layering issues, and
then months or years later having some innocuous change transform the
conceptual layering issue into a very real and painful link failure.
Usually this has happens at the least convenient time and requires quite a
bit of work to correct. Recently, I've been tracking and pushing back on
these conceptual violations because they *will* eventually cause concrete
and real problems. For example, how would this work in a modules world? If
the routine from BitcodeReader itself becomes inline?

As a more immediate and practical consideration, one way to check and
enforce layering is to do so at the header file and #include granularity.
Currently I do this for all of LLVM and Clang with a white list for
historical violations that haven't yet been fixed. I'd like to not add any
more exceptions by actually fixing the issues when they come up.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130321/dbb3c071/attachment.html>