<div dir="ltr">On Thu, Mar 21, 2013 at 8:30 PM, Andrew Trick <span dir="ltr"><<a href="mailto:atrick@apple.com" target="_blank" class="cremed">atrick@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="h5"><br><div><div>On Mar 21, 2013, at 8:23 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="cremed">chandlerc@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr">On Thu, Mar 21, 2013 at 8:20 PM, Andrew Trick <span dir="ltr"><<a href="mailto:atrick@apple.com" target="_blank" class="cremed">atrick@apple.com</a>></span> wrote:<br><div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>On Mar 21, 2013, at 7:24 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="cremed">chandlerc@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr">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.<div><br></div><div>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.</div>
<div><br></div><div>I think the correct solution is to do one of two things:</div><div><br></div><div>1) Create a new library which depends on both AsmParser and BitcodeReader and which can be used by opt and friends. Put IRReader here.</div>
<div>2) Fold AsmParser into the IR library (where AsmWriter already lives). Put IRReader into the BitcodeReader library.</div><div><br></div><div>I thought about a third option: put both AsmParser and IRReader into the BitcodeReader library. But increasingly that seems like a really gross layering.</div>
</div></blockquote><div><br></div></div><div>I like option #2, but some people might not want to link in the largish parser lib.</div></div></div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>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.</div>
</div></blockquote><div><br></div><div>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.</div>
</div></div></div>
</blockquote></div><br></div></div><div>Nothing would refer to BitcodeReader symbols except the inline function. Is there any real problem, or just conceptual?</div></div></blockquote><div><br></div><div style>... is the conceptual problem not enough?</div>
<div style><br></div><div style>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?</div>
<div style><br></div><div style>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.</div>
</div></div></div>