r228234 - [modules] When using -E, we may try to merge decls despite having no Sema
Ben Langmuir
blangmuir at apple.com
Mon Mar 23 12:51:27 PDT 2015
ping
> On Mar 14, 2015, at 9:25 AM, Ben Langmuir <blangmuir at apple.com> wrote:
>
> Hi Richard,
>
> This commit looks like it caused a regression in PCH-loading performance when the PCH imports modules. It doesn’t affect performance of loading modules on their own, or of a PCH that doesn’t use modules. If you have access to a Darwin system, it’s easy to reproduce with:
>
> $ cat t.h
> @import Cocoa;
>
> $ cat t.m
> // empty
>
> $ clang -fmodules -fmodules-cache-path=mcp -x objective-c-header t.h -o t.pch
> $ time clang -fmodules -fmodules-cache-path=mcp -Xclang -include-pch -Xclang t.pch t.m -fsyntax-only
>
> I’m seeing ~8x difference between r228233 and r228234. In real world code (where the .m file isn’t empty) we’ve seen up to a ~90% difference.
>
> Do you know what’s going on here? When a PCH loads I think we do it without Sema, but (at least previously) queued up the interesting Decls we saw for when Sema was added later. So I’m surprised this had such a big effect.
>
> Ben
>
>> On Feb 4, 2015, at 3:37 PM, Richard Smith <richard-llvm at metafoo.co.uk> wrote:
>>
>> Author: rsmith
>> Date: Wed Feb 4 17:37:59 2015
>> New Revision: 228234
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=228234&view=rev
>> Log:
>> [modules] When using -E, we may try to merge decls despite having no Sema
>> object. In such a case, use the TU's DC for merging global decls rather than
>> giving up when we find there is no TU scope.
>>
>> Ultimately, we should probably avoid all loading of decls when preprocessing,
>> but there are other reasonable use cases for loading an AST file with no Sema
>> object for which this is the right thing.
>>
>> Added:
>> cfe/trunk/test/Modules/Inputs/preprocess/
>> cfe/trunk/test/Modules/Inputs/preprocess/file.h
>> cfe/trunk/test/Modules/Inputs/preprocess/fwd.h
>> cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap
>> Modified:
>> cfe/trunk/include/clang/Frontend/CompilerInstance.h
>> cfe/trunk/lib/Frontend/CompilerInstance.cpp
>> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>> cfe/trunk/test/Modules/preprocess.m
>>
>> Modified: cfe/trunk/include/clang/Frontend/CompilerInstance.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CompilerInstance.h?rev=228234&r1=228233&r2=228234&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Frontend/CompilerInstance.h (original)
>> +++ cfe/trunk/include/clang/Frontend/CompilerInstance.h Wed Feb 4 17:37:59 2015
>> @@ -588,7 +588,7 @@ public:
>> /// Create an external AST source to read a PCH file.
>> ///
>> /// \return - The new object on success, or null on failure.
>> - static ExternalASTSource *createPCHExternalASTSource(
>> + static IntrusiveRefCntPtr<ASTReader> createPCHExternalASTSource(
>> StringRef Path, const std::string &Sysroot, bool DisablePCHValidation,
>> bool AllowPCHWithCompilerErrors, Preprocessor &PP, ASTContext &Context,
>> void *DeserializationListener, bool OwnDeserializationListener,
>>
>> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=228234&r1=228233&r2=228234&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)
>> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Wed Feb 4 17:37:59 2015
>> @@ -388,32 +388,30 @@ void CompilerInstance::createASTContext(
>> void CompilerInstance::createPCHExternalASTSource(
>> StringRef Path, bool DisablePCHValidation, bool AllowPCHWithCompilerErrors,
>> void *DeserializationListener, bool OwnDeserializationListener) {
>> - IntrusiveRefCntPtr<ExternalASTSource> Source;
>> bool Preamble = getPreprocessorOpts().PrecompiledPreambleBytes.first != 0;
>> - Source = createPCHExternalASTSource(
>> + ModuleManager = createPCHExternalASTSource(
>> Path, getHeaderSearchOpts().Sysroot, DisablePCHValidation,
>> AllowPCHWithCompilerErrors, getPreprocessor(), getASTContext(),
>> DeserializationListener, OwnDeserializationListener, Preamble,
>> getFrontendOpts().UseGlobalModuleIndex);
>> - ModuleManager = static_cast<ASTReader*>(Source.get());
>> - getASTContext().setExternalSource(Source);
>> }
>>
>> -ExternalASTSource *CompilerInstance::createPCHExternalASTSource(
>> +IntrusiveRefCntPtr<ASTReader> CompilerInstance::createPCHExternalASTSource(
>> StringRef Path, const std::string &Sysroot, bool DisablePCHValidation,
>> bool AllowPCHWithCompilerErrors, Preprocessor &PP, ASTContext &Context,
>> void *DeserializationListener, bool OwnDeserializationListener,
>> bool Preamble, bool UseGlobalModuleIndex) {
>> HeaderSearchOptions &HSOpts = PP.getHeaderSearchInfo().getHeaderSearchOpts();
>>
>> - std::unique_ptr<ASTReader> Reader;
>> - Reader.reset(new ASTReader(PP, Context,
>> - Sysroot.empty() ? "" : Sysroot.c_str(),
>> - DisablePCHValidation,
>> - AllowPCHWithCompilerErrors,
>> - /*AllowConfigurationMismatch*/false,
>> - HSOpts.ModulesValidateSystemHeaders,
>> - UseGlobalModuleIndex));
>> + IntrusiveRefCntPtr<ASTReader> Reader(
>> + new ASTReader(PP, Context, Sysroot.empty() ? "" : Sysroot.c_str(),
>> + DisablePCHValidation, AllowPCHWithCompilerErrors,
>> + /*AllowConfigurationMismatch*/ false,
>> + HSOpts.ModulesValidateSystemHeaders, UseGlobalModuleIndex));
>> +
>> + // We need the external source to be set up before we read the AST, because
>> + // eagerly-deserialized declarations may use it.
>> + Context.setExternalSource(Reader.get());
>>
>> Reader->setDeserializationListener(
>> static_cast<ASTDeserializationListener *>(DeserializationListener),
>> @@ -427,7 +425,7 @@ ExternalASTSource *CompilerInstance::cre
>> // Set the predefines buffer as suggested by the PCH reader. Typically, the
>> // predefines buffer will be empty.
>> PP.setPredefines(Reader->getSuggestedPredefines());
>> - return Reader.release();
>> + return Reader;
>>
>> case ASTReader::Failure:
>> // Unrecoverable failure: don't even try to process the input file.
>> @@ -442,6 +440,7 @@ ExternalASTSource *CompilerInstance::cre
>> break;
>> }
>>
>> + Context.setExternalSource(nullptr);
>> return nullptr;
>> }
>>
>>
>> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=228234&r1=228233&r2=228234&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
>> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Feb 4 17:37:59 2015
>> @@ -2591,6 +2591,11 @@ DeclContext *ASTDeclReader::getPrimaryCo
>> return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()
>> : nullptr;
>>
>> + // We can see the TU here only if we have no Sema object. In that case,
>> + // there's no TU scope to look in, so using the DC alone is sufficient.
>> + if (auto *TU = dyn_cast<TranslationUnitDecl>(DC))
>> + return TU;
>> +
>> return nullptr;
>> }
>>
>>
>> Added: cfe/trunk/test/Modules/Inputs/preprocess/file.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/file.h?rev=228234&view=auto
>> ==============================================================================
>> --- cfe/trunk/test/Modules/Inputs/preprocess/file.h (added)
>> +++ cfe/trunk/test/Modules/Inputs/preprocess/file.h Wed Feb 4 17:37:59 2015
>> @@ -0,0 +1,3 @@
>> +struct __FILE;
>> +#include "fwd.h"
>> +typedef struct __FILE FILE;
>>
>> Added: cfe/trunk/test/Modules/Inputs/preprocess/fwd.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/fwd.h?rev=228234&view=auto
>> ==============================================================================
>> --- cfe/trunk/test/Modules/Inputs/preprocess/fwd.h (added)
>> +++ cfe/trunk/test/Modules/Inputs/preprocess/fwd.h Wed Feb 4 17:37:59 2015
>> @@ -0,0 +1 @@
>> +struct __FILE;
>>
>> Added: cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap?rev=228234&view=auto
>> ==============================================================================
>> --- cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap (added)
>> +++ cfe/trunk/test/Modules/Inputs/preprocess/module.modulemap Wed Feb 4 17:37:59 2015
>> @@ -0,0 +1,2 @@
>> +module fwd { header "fwd.h" export * }
>> +module file { header "file.h" export * }
>>
>> Modified: cfe/trunk/test/Modules/preprocess.m
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/preprocess.m?rev=228234&r1=228233&r2=228234&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Modules/preprocess.m (original)
>> +++ cfe/trunk/test/Modules/preprocess.m Wed Feb 4 17:37:59 2015
>> @@ -1,9 +1,14 @@
>> // RUN: rm -rf %t
>> -// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -include %S/Inputs/preprocess-prefix.h -E %s | FileCheck -strict-whitespace %s
>> -// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -x objective-c-header -emit-pch %S/Inputs/preprocess-prefix.h -o %t.pch
>> -// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -include-pch %t.pch -E %s | FileCheck -strict-whitespace %s
>> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -include %S/Inputs/preprocess-prefix.h -E %s | FileCheck -strict-whitespace %s
>> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -x objective-c-header -emit-pch %S/Inputs/preprocess-prefix.h -o %t.pch
>> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -include-pch %t.pch -E %s | FileCheck -strict-whitespace %s
>> +//
>> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -x objective-c++ -include %S/Inputs/preprocess-prefix.h -E %s | FileCheck -strict-whitespace %s
>> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -x objective-c++-header -emit-pch %S/Inputs/preprocess-prefix.h -o %t.pch
>> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I %S/Inputs -I %S/Inputs/preprocess -x objective-c++ -include-pch %t.pch -E %s | FileCheck -strict-whitespace %s
>> #import "diamond_right.h"
>> #import "diamond_right.h" // to check that imports get their own line
>> +#include "file.h"
>> void test() {
>> top_left_before();
>> left_and_right();
>> @@ -15,6 +20,7 @@ void test() {
>>
>> // CHECK: @import diamond_right; /* clang -E: implicit import for "{{.*}}diamond_right.h" */{{$}}
>> // CHECK: @import diamond_right; /* clang -E: implicit import for "{{.*}}diamond_right.h" */{{$}}
>> +// CHECK: @import file; /* clang -E: implicit import for "{{.*}}file.h" */{{$}}
>> // CHECK-NEXT: void test() {{{$}}
>> // CHECK-NEXT: top_left_before();{{$}}
>> // CHECK-NEXT: left_and_right();{{$}}
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list