Ping. -- adrian
> On Apr 1, 2015, at 10:33 AM, Adrian Prantl <[email protected]> wrote: > > Hi Richard, > > 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? > > -- adrian > > <0001-Wrap-clang-module-files-in-a-Mach-O-ELF-or-COFF-cont.patch> > <0001-Adapt-clang-tools-extra-to-clang-module-format-chang.patch> > >> On Mar 9, 2015, at 4:32 PM, Adrian Prantl <[email protected] >> <mailto:[email protected]>> wrote: >> >> >>> On Mar 2, 2015, at 4:42 PM, Adrian Prantl <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> >>>> On Feb 25, 2015, at 5:30 PM, Richard Smith <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> >>>> On Wed, Feb 25, 2015 at 4:25 PM, Adrian Prantl <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> >>>>> On Feb 25, 2015, at 3:37 PM, Richard Smith <[email protected] >>>>> <mailto:[email protected]>> wrote: >>>>> >>>>> On Wed, Feb 25, 2015 at 2:22 PM, Adrian Prantl <[email protected] >>>>> <mailto:[email protected]>> wrote: >>>>> >>>>> > On Feb 25, 2015, at 1:34 PM, Richard Smith <[email protected] >>>>> > <mailto:[email protected]>> wrote: >>>>> > >>>>> > On Wed, Feb 25, 2015 at 1:02 PM, Adrian Prantl <[email protected] >>>>> > <mailto:[email protected]>> wrote: >>>>> > On Feb 24, 2015, at 6:28 PM, Adrian Prantl <[email protected] >>>>> > <mailto:[email protected]>> wrote: >>>>> >> You are correct. I need to remove the use of PCHGenerator from >>>>> >> ModuleContainerGenerator. >>>>> >>> On Feb 24, 2015, at 6:11 PM, NAKAMURA Takumi <[email protected] >>>>> >>> <mailto:[email protected]>> wrote: >>>>> >>> It still has circular dependencies between clangCodeGen and >>>>> >>> clangFrontend. >>>>> > >>>>> > After some deliberation, I noticed that there is actually more to be >>>>> > done here in order to resolve the cyclic dependencies. >>>>> > >>>>> > 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)? >>>>> >>>>> 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. >>>>> >>>>> "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. >>>> >>>> Challenge accepted, I should be possible: I image that we could introduce >>>> a ModuleContainerGeneratorFactory >>>> >>>> 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.) >>> >>> Hi Richard, >>> >>> 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. >>> 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. >>> >>> 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. >> >> Ping. No objections to threading a ModuleProvider through every >> CompilerInstance, or haven’t had a chance to look at it so far? >> 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. >> >> thanks, >> adrian >> >> <0001-Wrap-clang-module-files-in-a-Mach-O-ELF-or-COFF-cont.patch> >>> >>>> >>>> (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. >>>> >>>>> >>>>> > 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... >>>>> >>>>> Is that a hypothetical scenario or are there any such users out there? >>>>> >>>>> 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. >>>>> >>>>> 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? >>>>> >>>>> 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. >>>>> >>>>> > Here is a graph of (a simplified subset of) the dependencies after this >>>>> > commit: >>>>> > - Pretty much all of CodeGen depends on CodeGenOptions, which is >>>>> > currently part of Frontend. >>>>> > - BackendUtil and CodeGenAction depend on both CodeGen and Frontend. >>>>> > - CodeGenModuleContainer introduces a cyclic dependency between >>>>> > Frontend and CodeGen. >>>>> > >>>>> > <before.png> >>>>> > >>>>> > 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. >>>>> > >>>>> > The right place for CodeGenOptions is probably Basic, alongside >>>>> > LangOptions, TargetOptions, CommentOptions, etc. >>>>> >>>>> That sounds like a good idea; I might also be able to move it into >>>>> CodeGen itself. >>>>> > >>>>> > After this, the only remaining CodeGen->Frontend dependencies are >>>>> > CodeGen/BackendUtil.cpp and CodeGen/CodeGenAction.cpp: >>>>> > - CodeGenAction looks like it could safely be moved into FrontendTool, >>>>> > which is its only user. >>>>> > >>>>> > I don't think that's necessarily a good idea: CodeGenAction is tightly >>>>> > coupled to CodeGen and only very loosely coupled to the frontend. >>>>> >>>>> 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? >>>>> >>>>> 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. >>>> >>>> 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. >>>> >>>> I’ll try that. >>>> >>>> Thanks! >>>> >>>> -- adrian >>>> >>>>> >>>>> thanks for the feedback! >>>>> adrian >>>>> > >>>>> > - 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. >>>>> > The new picture then looks like this: >>>>> > >>>>> > <after.png> >>>>> > >>>>> > I’ll try and implement it this way; hopefully I didn’t miss any other >>>>> > edges in the graph. >>>>> > -- adrian >>>>> > >>>>> >> >>>>> >> thanks for noticing! >>>>> >> -- adrian >>>>> > >>>>> > >>>>> >>>>> >>>> >>>> >>> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] <mailto:[email protected]> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
