<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Ping.<div class=""><br class=""></div><div class="">-- adrian</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Apr 1, 2015, at 10:33 AM, Adrian Prantl <<a href="mailto:aprantl@apple.com" class="">aprantl@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><span style="font-family: monospace;" class="">Hi Richard,</span><br style="font-family: monospace;" class=""><br style="font-family: monospace;" class=""><font face="monospace" class="">Ping with rebased and cleaned up versions of the patches. Could you maybe just have a look at the way ModuleProvider is threaded through and see if there is anything about the design that you would feel uncomfortable about having to fix it in a more detailed post-commit review?</font><br style="font-family: monospace;" class=""><br style="font-family: monospace;" class=""><span style="font-family: monospace;" class="">-- adrian</span><div class=""><font face="monospace" class=""><br class=""></font><div class=""></div></div></div><span id="cid:28D54B9E-A9C4-4E30-B701-6233C8076537@apple.com"><0001-Wrap-clang-module-files-in-a-Mach-O-ELF-or-COFF-cont.patch></span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div class=""><div class=""></div></div></div><span id="cid:9726739A-85D9-43FE-B4C1-2D3C49A19939@apple.com"><0001-Adapt-clang-tools-extra-to-clang-module-format-chang.patch></span><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div class=""><font face="monospace" class=""><br class=""></font><div class=""><blockquote type="cite" class=""><div class="">On Mar 9, 2015, at 4:32 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" class="">aprantl@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Mar 2, 2015, at 4:42 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" class="">aprantl@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Feb 25, 2015, at 5:30 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" class="">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 25, 2015 at 4:25 PM, Adrian Prantl <span dir="ltr" class=""><<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Feb 25, 2015, at 3:37 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank" class="">richard@metafoo.co.uk</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 25, 2015 at 2:22 PM, Adrian Prantl <span dir="ltr" class=""><<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br class="">
> On Feb 25, 2015, at 1:34 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank" class="">richard@metafoo.co.uk</a>> wrote:<br class="">
><br class="">
> On Wed, Feb 25, 2015 at 1:02 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>> wrote:<br class="">
> On Feb 24, 2015, at 6:28 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>> wrote:<br class="">
>> You are correct. I need to remove the use of PCHGenerator from ModuleContainerGenerator.<br class="">
>>> On Feb 24, 2015, at 6:11 PM, NAKAMURA Takumi <<a href="mailto:geek4civic@gmail.com" target="_blank" class="">geek4civic@gmail.com</a>> wrote:<br class="">
>>> It still has circular dependencies between clangCodeGen and clangFrontend.<br class="">
><br class="">
> After some deliberation, I noticed that there is actually more to be done here in order to resolve the cyclic dependencies.<br class="">
><br class="">
> Well, it makes sense that there would be problems here, since you're coupling our lex-only / parse-only modes to IR generation. I wonder if it would be possible to entirely isolate Frontend from knowledge of the module wrapper format (putting it in FrontendTool instead)?<br class="">
<br class="">
</span>I was able to make PCHGenerator agnostic of the wrapper format, but it is not possible to push the wrapper-awareness out any further than FrontendActions.</blockquote><div class=""><br class=""></div><div class="">"not possible" is quite a strong claim to make. It would seem possible for FrontendTool to hand the CompilerInstance or Action a collection of callbacks for creating / importing module file data.</div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">Challenge accepted, I should be possible: I image that we could introduce a ModuleContainerGeneratorFactory</div></div></div></blockquote><div class=""><br class=""></div><div class="">Maybe the right level of interface is a ModuleProvider, so that CompilerInstance can just say "I want a representation of this Module to be loaded" and not care at all about whether or how that representation is built or stored on disk. (I can imagine distributed build systems that might want some behavior here other than physical files in some file system.)</div></div></div></div></div></blockquote><div class=""><br class=""></div><div class="">Hi Richard,</div><div class=""><br class=""></div><div class="">FYI, here is the patch with an abstract ModuleProvider interface that has one implementation in CodeGen/LLVMModuleProvider.cpp. The interface currently provides only two functions, one that creates an ASTConsumer that wraps a module in container and another function that unwraps a module container. The owner of the ModuleProvider is ModuleLoader, which is the base class of CompilerInstance and ASTUnit. Together with moving CodeGenOptions and BuryPointer into Basic, this made it possible to completely eliminate the dependency from Frontend -> CodeGen. As a bonus, clients are now able to provide alternative implementations of ModuleProvider as outlined above. In order for create something like the aforementioned distributed build system, some more functionality (such as the module cache handling) would need to move from Serialization into ModuleProvider.</div><div class="">The downside of the new approach is that now every tool now explicitly needs to instantiate a ModuleProvider which made writing this patch a little tedious. I’ve been thinking about providing a default argument for the module provider, but that would make it easy to accidentally mix-match incompatible module compilers, so I decided against that.</div><div class=""><br class=""></div><div class="">The diff between this and the previous version is quite large (though mostly mechanical changes) so I wanted to give you a chance to object before I push this in tree again.</div></div></div></div></blockquote><div class=""><br class=""></div>Ping. No objections to threading a ModuleProvider through every CompilerInstance, or haven’t had a chance to look at it so far?</div><div class="">In the mean time, I was able to simplify the patch some more: Because of abstract ModuleProvider interface, it is of course no longer necessary to shuffle CodeGenOptions, CodeGenAction, and BuryPointer around and they can stay where they are — which was part of the point of the whole exercise.</div><div class=""><br class=""></div><div class="">thanks,</div><div class="">adrian</div><div class=""><div class=""><br class=""></div><div class=""></div></div></div><span id="cid:F85E1384-7B75-4776-843E-B38083731D3F@apple.com" class=""><0001-Wrap-clang-module-files-in-a-Mach-O-ELF-or-COFF-cont.patch></span><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div class=""></div><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div class=""><br class=""></div></div></div><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></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" class=""><div class=""><div class="">(although I find this a little revolting ;-) that is passed to the CompilerInstance, which then hands it down to GenerateModuleAction/GeneratePCHAction, which asks the Factory to generate an new ModuleContainerGenerator instance which is actually an LLVModuleContainerGenerator. This way Frontend only needs to link against whereever the ModuleContainerGenerator base class is defined and each tool can decide which backend to use.</div><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> If not, I don't think we have any other option than to grow a Frontend->CodeGen dependency, which in turn will be terrible for people who want to use our frontend with a different backend...<br class="">
<br class="">
</span>Is that a hypothetical scenario or are there any such users out there?</blockquote><div class=""><br class=""></div><div class="">I don't have concrete knowledge of anyone doing this today, but enough people have asked about this that it's not unreasonable to think that someone might have actually done it.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I understand there are tools like clang-format/modernize/... which after this change have to link against the LLVM targets in order to generate clang-compatible modules; but are there any non-LLVM compilers that use clang as a frontend?</blockquote><div class=""><br class=""></div><div class="">I think there are, but I'm not sure if they all do this by converting LLVM IR to their own IR or whether some of them go straight from a Clang AST. Whether or not such compilers exist today, we should try to avoid creating barriers for people who want to do this in the future.</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> Here is a graph of (a simplified subset of) the dependencies after this commit:<br class="">
> - Pretty much all of CodeGen depends on CodeGenOptions, which is currently part of Frontend.<br class="">
> - BackendUtil and CodeGenAction depend on both CodeGen and Frontend.<br class="">
> - CodeGenModuleContainer introduces a cyclic dependency between Frontend and CodeGen.<br class="">
><br class="">
</span>> <before.png><br class="">
<span class="">><br class="">
> The above cycle can be resolved by reversing the CodeGen->Frontend dependency and splitting out the common dependencies CodeGenOptions and frontend::utils::BuryPointer into a separate library that I’m calling FrontendSupport for lack of a better name.<br class="">
><br class="">
> The right place for CodeGenOptions is probably Basic, alongside LangOptions, TargetOptions, CommentOptions, etc.<br class="">
<br class="">
</span>That sounds like a good idea; I might also be able to move it into CodeGen itself.<br class="">
<span class="">><br class="">
> After this, the only remaining CodeGen->Frontend dependencies are CodeGen/BackendUtil.cpp and CodeGen/CodeGenAction.cpp:<br class="">
> - CodeGenAction looks like it could safely be moved into FrontendTool, which is its only user.<br class="">
><br class="">
> I don't think that's necessarily a good idea: CodeGenAction is tightly coupled to CodeGen and only very loosely coupled to the frontend.<br class="">
<br class="">
</span>I agree that it is tightly coupled to CodeGen, but it is also tightly coupled to CompilerInstance, which has its tentacles all over Frontend so it needs to move somewhere to break the cycle. Do you see any specific problems with having CodeGenAction in FrontendTool?<br class=""></blockquote><div class=""><br class=""></div><div class="">No specific problem, only a general malaise: we deliberately isolate all the parts of Clang that touch LLVM IR in lib/CodeGen; this change would violate that notional encapsulation.</div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">Okay, separating out everything that touches IR that makes sense. I think it should be possible to remove all references to CompilerInstance from CodeGenAction; it appears to be only used as a holder of the various options and the DiagnosticsEngine, which would allow us to leave it in CodeGen.</div><div class=""><br class=""></div><div class="">I’ll try that.</div></div></div></blockquote><div class=""><br class=""></div><div class="">Thanks!</div><div class=""> </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" class=""><div class=""><span class="HOEnZb"><font color="#888888" class=""><div class="">-- adrian</div></font></span><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
thanks for the feedback!<br class="">
adrian<br class="">
<span class="">><br class="">
> - BackendUtil can stay were it is, it is needed by CodeGenAction and (via CodeGenModuleContainer) by Frontend. The dependency on Frontend can be eliminated by splitting BuryPointer out from Utils.<br class="">
> The new picture then looks like this:<br class="">
><br class="">
</span>> <after.png><br class="">
<div class=""><div class="">><br class="">
> I’ll try and implement it this way; hopefully I didn’t miss any other edges in the graph.<br class="">
> -- adrian<br class="">
><br class="">
>><br class="">
>> thanks for noticing!<br class="">
>> -- adrian<br class="">
><br class="">
><br class="">
<br class="">
</div></div></blockquote></div><br class=""></div></div>
</div></blockquote></span></div><br class=""></div></blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></div>_______________________________________________<br class="">cfe-commits mailing list<br class=""><a href="mailto:cfe-commits@cs.uiuc.edu" class="">cfe-commits@cs.uiuc.edu</a><br class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" class="">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br class=""></div></blockquote></div><br class=""></div></div></div>_______________________________________________<br class="">cfe-commits mailing list<br class=""><a href="mailto:cfe-commits@cs.uiuc.edu" class="">cfe-commits@cs.uiuc.edu</a><br class="">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br class=""></div></blockquote></div><br class=""></div></body></html>