[clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records #69975

jansvoboda11 · 2023-10-23T21:48:21Z

Deserialization of the DIAGNOSTIC_OPTIONS and HEADER_SEARCH_PATHS records is slow and done for every transitively loaded PCM. Deserialization of these records cannot be skipped, because the words are VBR6-encoded and we don't store the length of the entire record. We could either turn them into binary blobs that can be skipped during deserialization, or skip writing them altogether. This patch takes the latter approach, since these records are not necessary in scanning PCMs. The scanner doesn't make any guarantees about the accuracy of diagnostics, and we always have the same header search paths due to strict context hashing.

The commit that makes the DIAGNOSTIC_OPTIONS record skippable was originally implemented by @benlangmuir in a downstream repo.

llvmbot · 2023-10-23T21:50:31Z

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)

Changes

Deserialization of the DIAGNOSTIC_OPTIONS and HEADER_SEARCH_PATHS records is slow and done for every transitively loaded PCM. Deserialization of these records cannot be skipped, because the words are VBR6-encoded and we don't store the length of the entire record. We could either turn them into binary blobs that can be skipped during deserialization, or skip writing them altogether. This patch takes the latter approach, since these records are not necessary in scanning PCMs. We don't make any guarantees about diagnostics reported by the scanner, and we always have the same header search paths due to strict context hashing.

Full diff: https://github.com/llvm/llvm-project/pull/69975.diff

5 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+4)
(modified) clang/include/clang/Lex/HeaderSearchOptions.h (+9-1)
(modified) clang/lib/Serialization/ASTWriter.cpp (+26-24)
(modified) clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp (+2)
(added) clang/test/Modules/diagnostic-options-mismatch.c (+41)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 5415b18d3f406df..68ec32bdf56a555 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2944,6 +2944,10 @@ def fno_modules_validate_textual_header_includes :
   MarshallingInfoNegativeFlag<LangOpts<"ModulesValidateTextualHeaderIncludes">>,
   HelpText<"Do not enforce -fmodules-decluse and private header restrictions for textual headers. "
            "This flag will be removed in a future Clang release.">;
+defm modules_skip_diagnostic_options : BoolFOption<"modules-skip-diagnostic-options",
+    HeaderSearchOpts<"ModulesSkipDiagnosticOptions">, DefaultFalse,
+    PosFlag<SetTrue, [], [], "Disable writing diagnostic options">,
+    NegFlag<SetFalse>, BothFlags<[], [CC1Option]>>;
 
 def fincremental_extensions :
   Flag<["-"], "fincremental-extensions">,
diff --git a/clang/include/clang/Lex/HeaderSearchOptions.h b/clang/include/clang/Lex/HeaderSearchOptions.h
index c7d95006bb779ad..766b1919a5b5eab 100644
--- a/clang/include/clang/Lex/HeaderSearchOptions.h
+++ b/clang/include/clang/Lex/HeaderSearchOptions.h
@@ -219,6 +219,12 @@ class HeaderSearchOptions {
 
   unsigned ModulesValidateDiagnosticOptions : 1;
 
+  /// Whether to entirely skip writing diagnostic options.
+  unsigned ModulesSkipDiagnosticOptions : 1;
+
+  /// Whether to entirely skip writing header search paths.
+  unsigned ModulesSkipHeaderSearchPaths : 1;
+
   unsigned ModulesHashContent : 1;
 
   /// Whether we should include all things that could impact the module in the
@@ -238,7 +244,9 @@ class HeaderSearchOptions {
         ModulesValidateSystemHeaders(false),
         ValidateASTInputFilesContent(false),
         ForceCheckCXX20ModulesInputFiles(false), UseDebugInfo(false),
-        ModulesValidateDiagnosticOptions(true), ModulesHashContent(false),
+        ModulesValidateDiagnosticOptions(true),
+        ModulesSkipDiagnosticOptions(false),
+        ModulesSkipHeaderSearchPaths(false), ModulesHashContent(false),
         ModulesStrictContextHash(false) {}
 
   /// AddPath - Add the \p Path path to the specified \p Group list.
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 27700c711d52fdd..d5689cb3c97cb39 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1212,9 +1212,12 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor &PP,
     Record.clear();
   }
 
+  const auto &HSOpts = PP.getHeaderSearchInfo().getHeaderSearchOpts();
+
   // Diagnostic options.
   const auto &Diags = Context.getDiagnostics();
   const DiagnosticOptions &DiagOpts = Diags.getDiagnosticOptions();
+  if (!HSOpts.ModulesSkipDiagnosticOptions) {
 #define DIAGOPT(Name, Bits, Default) Record.push_back(DiagOpts.Name);
 #define ENUM_DIAGOPT(Name, Type, Bits, Default)                                \
   Record.push_back(static_cast<unsigned>(DiagOpts.get##Name()));
@@ -1229,35 +1232,34 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor &PP,
   // are generally transient files and will almost always be overridden.
   Stream.EmitRecord(DIAGNOSTIC_OPTIONS, Record);
   Record.clear();
+  }
 
   // Header search paths.
-  Record.clear();
-  const HeaderSearchOptions &HSOpts =
-      PP.getHeaderSearchInfo().getHeaderSearchOpts();
-
-  // Include entries.
-  Record.push_back(HSOpts.UserEntries.size());
-  for (unsigned I = 0, N = HSOpts.UserEntries.size(); I != N; ++I) {
-    const HeaderSearchOptions::Entry &Entry = HSOpts.UserEntries[I];
-    AddString(Entry.Path, Record);
-    Record.push_back(static_cast<unsigned>(Entry.Group));
-    Record.push_back(Entry.IsFramework);
-    Record.push_back(Entry.IgnoreSysRoot);
-  }
+  if (!HSOpts.ModulesSkipHeaderSearchPaths) {
+    // Include entries.
+    Record.push_back(HSOpts.UserEntries.size());
+    for (unsigned I = 0, N = HSOpts.UserEntries.size(); I != N; ++I) {
+      const HeaderSearchOptions::Entry &Entry = HSOpts.UserEntries[I];
+      AddString(Entry.Path, Record);
+      Record.push_back(static_cast<unsigned>(Entry.Group));
+      Record.push_back(Entry.IsFramework);
+      Record.push_back(Entry.IgnoreSysRoot);
+    }
 
-  // System header prefixes.
-  Record.push_back(HSOpts.SystemHeaderPrefixes.size());
-  for (unsigned I = 0, N = HSOpts.SystemHeaderPrefixes.size(); I != N; ++I) {
-    AddString(HSOpts.SystemHeaderPrefixes[I].Prefix, Record);
-    Record.push_back(HSOpts.SystemHeaderPrefixes[I].IsSystemHeader);
-  }
+    // System header prefixes.
+    Record.push_back(HSOpts.SystemHeaderPrefixes.size());
+    for (unsigned I = 0, N = HSOpts.SystemHeaderPrefixes.size(); I != N; ++I) {
+      AddString(HSOpts.SystemHeaderPrefixes[I].Prefix, Record);
+      Record.push_back(HSOpts.SystemHeaderPrefixes[I].IsSystemHeader);
+    }
 
-  // VFS overlay files.
-  Record.push_back(HSOpts.VFSOverlayFiles.size());
-  for (StringRef VFSOverlayFile : HSOpts.VFSOverlayFiles)
-    AddString(VFSOverlayFile, Record);
+    // VFS overlay files.
+    Record.push_back(HSOpts.VFSOverlayFiles.size());
+    for (StringRef VFSOverlayFile : HSOpts.VFSOverlayFiles)
+      AddString(VFSOverlayFile, Record);
 
-  Stream.EmitRecord(HEADER_SEARCH_PATHS, Record);
+    Stream.EmitRecord(HEADER_SEARCH_PATHS, Record);
+  }
 
   // Write out the diagnostic/pragma mappings.
   WritePragmaDiagnosticMappings(Diags, /* isModule = */ WritingModule);
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 29df0c3a0afdb5c..dc7bb289cf8f905 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -252,6 +252,8 @@ class DependencyScanningAction : public tooling::ToolAction {
     // TODO: Implement diagnostic bucketing to reduce the impact of strict
     // context hashing.
     ScanInstance.getHeaderSearchOpts().ModulesStrictContextHash = true;
+    ScanInstance.getHeaderSearchOpts().ModulesSkipDiagnosticOptions = true;
+    ScanInstance.getHeaderSearchOpts().ModulesSkipHeaderSearchPaths = true;
 
     // Avoid some checks and module map parsing when loading PCM files.
     ScanInstance.getPreprocessorOpts().ModulesCheckRelocated = false;
diff --git a/clang/test/Modules/diagnostic-options-mismatch.c b/clang/test/Modules/diagnostic-options-mismatch.c
new file mode 100644
index 000000000000000..cdd6f897729a9d8
--- /dev/null
+++ b/clang/test/Modules/diagnostic-options-mismatch.c
@@ -0,0 +1,41 @@
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: split-file %s %t
+
+//--- module.modulemap
+module Mod { header "mod.h" }
+//--- mod.h
+//--- tu.c
+#include "mod.h"
+
+// Without any extra compiler flags, mismatched diagnostic options trigger recompilation of modules.
+//
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache1 -fdisable-module-hash \
+// RUN:   -fsyntax-only %t/tu.c -Wnon-modular-include-in-module
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache1 -fdisable-module-hash \
+// RUN:   -fsyntax-only %t/tu.c -Werror=non-modular-include-in-module -Rmodule-build 2>&1 \
+// RUN:   | FileCheck %s --check-prefix=DID-REBUILD
+// DID-REBUILD: remark: building module 'Mod'
+
+// When skipping serialization of diagnostic options, mismatches cannot be detected, old PCM file gets reused.
+//
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache2 -fdisable-module-hash \
+// RUN:   -fsyntax-only %t/tu.c -fmodules-skip-diagnostic-options -Wnon-modular-include-in-module
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache2 -fdisable-module-hash \
+// RUN:   -fsyntax-only %t/tu.c -fmodules-skip-diagnostic-options -Werror=non-modular-include-in-module -Rmodule-build 2>&1 \
+// RUN:   | FileCheck %s --check-prefix=DID-REUSE --allow-empty
+// DID-REUSE-NOT: remark: building module 'Mod'
+//
+// RUN: %clang_cc1 -module-file-info %t/cache2/Mod.pcm | FileCheck %s --check-prefix=NO-DIAG-OPTS
+// NO-DIAG-OPTS-NOT: Diagnostic flags:
+
+// When disabling validation of diagnostic options, mismatches are not checked for, old PCM file gets reused.
+//
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache3 -fdisable-module-hash \
+// RUN:   -fsyntax-only %t/tu.c -fmodules-disable-diagnostic-validation -Wnon-modular-include-in-module
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache3 -fdisable-module-hash \
+// RUN:   -fsyntax-only %t/tu.c -fmodules-disable-diagnostic-validation -Werror=non-modular-include-in-module -Rmodule-build 2>&1 \
+// RUN:   | FileCheck %s --check-prefix=DID-REUSE --allow-empty
+//
+// RUN: %clang_cc1 -module-file-info %t/cache3/Mod.pcm | FileCheck %s --check-prefix=OLD-DIAG-OPTS
+// OLD-DIAG-OPTS: Diagnostic flags:
+// OLD-DIAG-OPTS-NEXT: -Wnon-modular-include-in-module

clang/include/clang/Driver/Options.td

clang/lib/Serialization/ASTWriter.cpp

ChuanqiXu9

Besides deps scanning, have you tried to enable these option to compile with clang modules? I mean, if it is safe to enable such options at compilation times, it looks a valid optimization for C++20 modules too. If it is not safe to do so, I think we need to document it explicitly.

jansvoboda11 · 2023-10-25T22:36:08Z

Besides deps scanning, have you tried to enable these option to compile with clang modules? I mean, if it is safe to enable such options at compilation times, it looks a valid optimization for C++20 modules too.

I did not. I suspect it won't show in profiles of compiles the same way it does for scans, where this kind of overhead gets exacerbated.

If it is not safe to do so, I think we need to document it explicitly.

This is a -cc1-only flag, so I don't think documenting the nuances/implications is super important.

clang/include/clang/Lex/HeaderSearchOptions.h

jansvoboda11 · 2023-10-31T16:55:59Z

clang/lib/Serialization/ASTWriter.cpp

  // Diagnostic options.
  const auto &Diags = Context.getDiagnostics();
  const DiagnosticOptions &DiagOpts = Diags.getDiagnosticOptions();
+  if (!HSOpts.ModulesSkipDiagnosticOptions) {
 #define DIAGOPT(Name, Bits, Default) Record.push_back(DiagOpts.Name);
 #define ENUM_DIAGOPT(Name, Type, Bits, Default)                                \
  Record.push_back(static_cast<unsigned>(DiagOpts.get##Name()));


clang-format unfortunately does not indent this line.

@benlangmuir

Deserialization of the `DIAGNOSTIC_OPTIONS` and `HEADER_SEARCH_PATHS` records is slow and done for every transitively loaded PCM. Deserialization of these records cannot be skipped, because the words are VBR6-encoded and we don't store the length of the entire record. We could either turn them into binary blobs that can be skipped during deserialization, or skip writing them altogether. This patch takes the latter approach, since these records are not necessary in scanning PCMs. The scanner doesn't make any guarantees about the accuracy of diagnostics, and we always have the same header search paths due to strict context hashing. The commit that makes the `DIAGNOSTIC_OPTIONS` record skippable was originally implemented by @benlangmuir in a downstream repo. (cherry picked from commit 6c465a2)

Following up on #69975, this patch skips writing `DIAG_PRAGMA_MAPPINGS` as well. Deserialization of this PCM record is still showing up in profiles, since it needs to be VBR-decoded for every transitively loaded PCM file. The scanner doesn't make any guarantees about diagnostic accuracy (and it even disables all warnings), so skipping this record should be safe.

Following up on llvm#69975, this patch skips writing `DIAG_PRAGMA_MAPPINGS` as well. Deserialization of this PCM record is still showing up in profiles, since it needs to be VBR-decoded for every transitively loaded PCM file. The scanner doesn't make any guarantees about diagnostic accuracy (and it even disables all warnings), so skipping this record should be safe. (cherry picked from commit 22c6851)

Following up on llvm#69975, this patch skips writing `DIAG_PRAGMA_MAPPINGS` as well. Deserialization of this PCM record is still showing up in profiles, since it needs to be VBR-decoded for every transitively loaded PCM file. The scanner doesn't make any guarantees about diagnostic accuracy (and it even disables all warnings), so skipping this record should be safe.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Oct 23, 2023

jansvoboda11 requested a review from benlangmuir October 23, 2023 21:51

benlangmuir reviewed Oct 23, 2023

View reviewed changes

clang/include/clang/Driver/Options.td Show resolved Hide resolved

clang/lib/Serialization/ASTWriter.cpp Show resolved Hide resolved

ChuanqiXu9 reviewed Oct 24, 2023

View reviewed changes

jansvoboda11 requested a review from benlangmuir October 25, 2023 22:36

ChuanqiXu9 reviewed Oct 26, 2023

View reviewed changes

clang/include/clang/Lex/HeaderSearchOptions.h Show resolved Hide resolved

jansvoboda11 commented Oct 31, 2023

View reviewed changes

benlangmuir approved these changes Oct 31, 2023

View reviewed changes

jansvoboda11 added 6 commits November 2, 2023 14:54

[clang][modules] Make DIAGNOSTIC_OPTIONS skippable

5dd9d64

[clang][modules] Make HEADER_SEARCH_PATHS skippable

69d3645

[clang][deps] Skip DIAGNOSTIC_OPTIONS and HEADER_SEARCH_PATHS

3ce52bf

Add -fmodules-skip-header-search-paths test, run clang-format

7be1ff5

Note new options are for the scanner

8811663

Fix test on Windows

6283999

jansvoboda11 force-pushed the skip-slow-pcm-records branch from 3ae8af2 to 6283999 Compare November 2, 2023 22:06

jansvoboda11 merged commit 6c465a2 into llvm:main Nov 2, 2023

jansvoboda11 deleted the skip-slow-pcm-records branch November 2, 2023 22:08

jansvoboda11 mentioned this pull request Nov 10, 2023

[clang][deps] Skip writing DIAG_PRAGMA_MAPPINGS record #70874

Merged

jansvoboda11 mentioned this pull request Nov 10, 2023

[clang][deps] Skip writing DIAG_PRAGMA_MAPPINGS record swiftlang/llvm-project#7776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records #69975

[clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records #69975

Uh oh!

jansvoboda11 commented Oct 23, 2023 •

edited

Loading

Uh oh!

llvmbot commented Oct 23, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

ChuanqiXu9 left a comment

Uh oh!

jansvoboda11 commented Oct 25, 2023

Uh oh!

Uh oh!

jansvoboda11 Oct 31, 2023

Uh oh!

Uh oh!

[clang][deps] Skip slow UNHASHED_CONTROL_BLOCK records #69975

[clang][deps] Skip slow UNHASHED_CONTROL_BLOCK records #69975

Uh oh!

Conversation

jansvoboda11 commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Uh oh!

jansvoboda11 commented Oct 25, 2023

Uh oh!

Uh oh!

jansvoboda11 Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records #69975

[clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records #69975

jansvoboda11 commented Oct 23, 2023 •

edited

Loading

llvmbot commented Oct 23, 2023 •

edited

Loading