Page MenuHomePhabricator

[clang-cl] Don't emit dllexport inline functions etc. from pch files (PR37801)
ClosedPublic

Authored by hans on Jun 21 2018, 6:19 AM.

Details

Summary

With MSVC, PCH files are created along with an object file that needs to be linked into the final library or executable. That object file contains the code generated when building the headers. In particular, it will include definitions of inline dllexport functions, and because they are emitted in this object file, other files using the PCH do not need to emit them. See the bug for an example.

This patch makes clang-cl match MSVC's behaviour in this regard, causing significant compile-time savings when building dlls using precompiled headers.

For example, in a 64-bit optimized shared library build of Chromium with PCH, it reduces the binary size and compile time of stroke_opacity_custom.obj from 9315564 bytes to 3659629 bytes and 14.6 to 6.63 s. The wall-clock time of building blink_core.dll goes from 38m41s to 22m33s. ("user" time goes from 1979m to 1142m).

Diff Detail

Event Timeline

hans created this revision.Jun 21 2018, 6:19 AM
thakis accepted this revision.Jun 21 2018, 6:46 AM

Looks like pretty straightforward plumbing. I can't think of a better place than ASTContext either. I think this is fine with your two XXXs just removed.

Exciting build time wins!

lib/AST/ASTContext.cpp
9557

nit: s/be/have/

This revision is now accepted and ready to land.Jun 21 2018, 6:46 AM
hans added a comment.Jun 21 2018, 6:57 AM

I think you revved the PCH version last. Did you have to do anything special, e.g. will it break vendors who ship PCH files as part of SDKs and such? Or are things generally set up to handle this?

PCHs aren't compatible with themselves if only the compiler revision changes, so I'm not sure changing that field should be worse than a regular compiler revision update (which happens at every commit). But I don't know what this field is for. I don't remember any trouble from me changing it though, and if it was bad to change it I'd hope there'd be a comment right above the field telling us why. (If I had run into problems, I would've expected me to add a comment like this.)

hans added a comment.Jun 21 2018, 8:18 AM

PCHs aren't compatible with themselves if only the compiler revision changes, so I'm not sure changing that field should be worse than a regular compiler revision update (which happens at every commit). But I don't know what this field is for. I don't remember any trouble from me changing it though, and if it was bad to change it I'd hope there'd be a comment right above the field telling us why. (If I had run into problems, I would've expected me to add a comment like this.)

Sounds good, thanks.

thakis added inline comments.Jun 21 2018, 10:19 AM
lib/AST/ASTContext.cpp
9563

It'd be good to add a comment (and a test) explaining why / how _referenced_ inline functions still get emitted for inlining.

rnk added a subscriber: dblaikie.Jun 21 2018, 1:30 PM

LangOpts.ModulesCodegen is very related in spirit to this, but I think we need a distinct option because that was designed to handle all inline functions (too much), not just dllexport inline functions. + @dblaikie

include/clang/AST/ASTContext.h
2886–2887 ↗(On Diff #152267)

Does LangOpts.CompilingPCH do the right thing, or is that also true when we make a PCH with no object file? In any case, I'd probably make this a langopt. Even if it's functionally different from ModulesCodegen and ModulesDebugInfo, it's the same basic idea.

In D48426#1139823, @rnk wrote:

LangOpts.ModulesCodegen is very related in spirit to this, but I think we need a distinct option because that was designed to handle all inline functions (too much), not just dllexport inline functions. + @dblaikie

Does it have to be only the dllexported inline functions - or could this be used to home all inline functions? (I guess not - presumably it's not acceptable for the compiler to implicitly promote something to dllexport (& if it doesn't do that promotion, then the function wouldn't be visible from the use))

Is it valid to provide a definition for optimization purposes (LLVM's available_externally linkage) for these inline functions? Or is it required that, after a change to the header (modifying the implementation of one of these dllexported inline functions), the DLL they're exported could be rebuilt without rebuilding the clients & the change in behavior would be correctly applied?

test/CodeGen/pch-dllexport.cpp
23

This is a pretty specific "NOT" - maybe:

define {{.*}}@"?bar@...

would be better to ensure no definition is provided? (similarly above)

Also, should this file have some non-exported inline functions to check those do the right thing?

hans marked 4 inline comments as done.Jun 22 2018, 2:58 AM
In D48426#1139823, @rnk wrote:

LangOpts.ModulesCodegen is very related in spirit to this, but I think we need a distinct option because that was designed to handle all inline functions (too much), not just dllexport inline functions. + @dblaikie

Does it have to be only the dllexported inline functions - or could this be used to home all inline functions? (I guess not - presumably it's not acceptable for the compiler to implicitly promote something to dllexport (& if it doesn't do that promotion, then the function wouldn't be visible from the use))

Right, I don't think we could do that. But regular inline functions isn't as large a problem as the dllexport ones, which we have to emit even if they're not referenced, causing a huge amount of redundant codegen.

Is it valid to provide a definition for optimization purposes (LLVM's available_externally linkage) for these inline functions? Or is it required that, after a change to the header (modifying the implementation of one of these dllexported inline functions), the DLL they're exported could be rebuilt without rebuilding the clients & the change in behavior would be correctly applied?

Yes, we will provide such definitions (not just available_externally, but weak_odr) if the inline function is referenced, as usual. This is only homing unreferenced but "force emitted", i.e. DeclMustBeEmitted, functions.

include/clang/AST/ASTContext.h
2886–2887 ↗(On Diff #152267)

Yes, CompilingPCH is always set when building PCHs, also when there's no object file so we can't use that.

Moving BuildingPCHWithObjectFile to LangOpts sounds good to me.

Actually that's interesting, because it means the flag will get written into the PCH will all the other LangOpts, and we could in theory look for that in ASTReader. But it seems the code isn't really set up to store the LangOpts coming out of an AST file, we only verify that they're compatible.

test/CodeGen/pch-dllexport.cpp
23

Updated the -NOT checks and added test.

hans updated this revision to Diff 152444.Jun 22 2018, 3:00 AM
hans marked 2 inline comments as done.

Addressing review comments.

thakis added inline comments.Jun 22 2018, 5:25 AM
test/CodeGen/pch-dllexport.cpp
42

There still isn't a non-dllexported-but-referenced inline function in here as far as I can see. Am I just missing it?

hans added a comment.Jun 22 2018, 7:31 AM

I hit a snag while building some more Chromium targets. For class templates with explicit instantiation decls in the PCH file and explicit instantiation definitions in a .cc file, the function definition will be marked as coming from the PCH, even though it wasn't defined there. For example:

#ifndef IN_HEADER
#define IN_HEADER

template <typename T> struct Template { Template() {} };
extern template class Template<int>;

#else

template class __declspec(dllexport) Template<int>;

#endif

This isn't a problem for regular functions where we build a decl chain: the first declaration may come from a PCH, but the definition would be a separate Decl object which does not.

I'm trying to figure out how to handle this.

hans updated this revision to Diff 152659.Jun 25 2018, 4:42 AM

Added special-casing for explicit template instantiations, and missing test case suggested by Nico.

Please take another look.

Still looks good, ship it! One more suggestion about additional test coverage (but maybe it's already there and I'm just missing it).

test/CodeGen/pch-dllexport.cpp
55

This has two interesting cases:

  1. An explicit instantiation declaration.
  2. An explicit instantiation definition.

You have a test for 2, but not for 1, as far as I can tell (?). For 1, the inline function from the pch-obj should be used (which probably already works; if not a FIXME is enough).

hans added inline comments.Jun 25 2018, 5:52 AM
test/CodeGen/pch-dllexport.cpp
55

It's really testing the "explicit instantiation def after explicit instantiation decl" case.

With just an explicit instantiation decl (1), there's not much to test, since that's like a promise that someone else is going to instantiate the template. Nothing needs to be emitted for that.

Maybe I'm not understanding your question?

thakis added inline comments.Jun 25 2018, 5:59 AM
test/CodeGen/pch-dllexport.cpp
55

Nothing needs to be emitted for that.

Right, I meant including a test that we really don't emit anything for that. But feel free to omit it if you don't think it adds much. I suggested it because we end up with TSK_ExplicitInstantiationDeclaration in that case, and you're checking the TSK in this change, and you have coverage for the other kinds.

This revision was automatically updated to reflect the committed changes.