On Wed, Jul 15, 2015 at 1:51 PM, Adrian Prantl <apra...@apple.com> wrote:
> > On Jul 15, 2015, at 1:16 PM, Richard Smith <rich...@metafoo.co.uk> wrote: > > On Wed, Jul 15, 2015 at 12:54 PM, Adrian Prantl <apra...@apple.com> wrote: > >> Here is a patch that implements a -fmodule-format=[obj,raw] option. I >> have not yet implemented adding the module format to the module hash or >> filename. >> > > Generally, we only have a -cc1 flag to switch to the non-default state. > > > Okay I think it’s better then to bake the default into cc1 rather than the > driver. > > > >> The default setting is obj (based on the assumption that this is most >> beneficial to platforms with a shared module cache) and the driver adds an >> explicit -fmodule-format=raw on Linux. >> > > I think this is backwards; I think putting more stuff into the precompiled > module format should be opt-in rather than opt-out. > > I am not sure whether having the driver emit this option for Linux is a >> good idea: Are explicit module builds a feature of the Google build system, >> or are they a Linux platform feature? >> > > Neither; they're a Clang feature. Implicit module builds are a > compatibility feature for legacy build systems, and should be avoided > wherever possible because they introduce a performance hit, interact poorly > with distributed builds, behave badly if the cache gets cleared (especially > once we put debug info in modules), and so on. > > Currently the only way to enable obj-wrapped modules on Linux is to pass a >> cc1 option. Even with -fmodule-format=raw specified, clang can still read >> obj-wrapped modules. >> > > OK, but presumably we'll add -gmodules or similar at some point to resolve > that issue. > > > Yes. > > My updated plan is to make =raw the default. The driver expands > “-gmodules" into something like "-dwarf-ext-typerefs[TBD] > -fmodule-format=obj”. Having =raw be the default is safe as long as we > encode the module format into the module hash so the raw and obj versions > don’t conflict. Once we have a pass to translate module ast->obj as you > would do for explicit builds, we can make more fine-grained adjustments to > this. > > > I’m not in love with the actual implementation, so suggestions and >> feedback are very welcome! >> > > I assume the "if (1 || ..." was not intentional? > > > Yes :-) I had that in there to run the testsuite with the different > default settings. > > > It doesn't seem ideal to have the top-level driver create the > wrapper-format handler, and then ignore that from the frontend code. That's > also not going to scale to module formats other than obj and raw. Is there > any other way we can deal with this without breaking the layering? > > > That’s exactly what bothered me. The wrapper-format handler is primarily > there so Frontend does not need to depend on CodeGen. Currently we’re > passing it in to CompilerInstance at creation time (which happens at the > very top level). It might be possible to solve this a little more elegantly > by having a > > class PCHContainerOperationsRegistry { > void registerPCHContainerOperations(PCHContainerOperations *Handler); > shared_ptr<PCHContainerOperations> getPCHContainerOperations(PCHFormat > Format); > } > > have the toplevel register RawPCHContainerOperations and > ObjectFilePCHContainerOperations and have > CompilerInstance::getPCHContainerOperations() return the appropriate > handler for the currently registered CodeGenOptions (since the caller > usually doesn’t have access to the CodeGenOptions). > Yes, something along those lines would make sense to me. I was thinking of a slightly different design, more like class ASTContainerFormatHandler { PCHContainerOperations *getFormat(StringRef Format) = 0; }; ... but I'll leave the details up to you. Maybe this would also be a good time to think about the writer / reader split (for consumers that want to read object-file-wrapped ASTs but don't want to link against the LLVM code generator)?
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits