Ping? -- adrian > On Apr 17, 2015, at 12:04 PM, Adrian Prantl <[email protected]> wrote: > > Ping. > > -- adrian > >> On Apr 1, 2015, at 10:33 AM, Adrian Prantl <[email protected] >> <mailto:[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 >>> <http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits> >> >> _______________________________________________ >> 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
