[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-11-02 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 closed 
https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-11-02 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 updated 
https://github.com/llvm/llvm-project/pull/69975

>From 5dd9d64726dba95f71dfb276dd1be4d386313a99 Mon Sep 17 00:00:00 2001
From: Jan Svoboda 
Date: Tue, 10 Oct 2023 14:16:13 -0700
Subject: [PATCH 1/6] [clang][modules] Make `DIAGNOSTIC_OPTIONS` skippable

---
 clang/include/clang/Driver/Options.td |  4 ++
 clang/include/clang/Lex/HeaderSearchOptions.h |  6 ++-
 clang/lib/Serialization/ASTWriter.cpp |  4 ++
 .../Modules/diagnostic-options-mismatch.c | 41 +++
 4 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/Modules/diagnostic-options-mismatch.c

diff --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index fcf6a4b2ccb2369..b34c68f7f238749 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2947,6 +2947,10 @@ def fno_modules_validate_textual_header_includes :
   
MarshallingInfoNegativeFlag>,
   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,
+NegFlag, 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..d6dd94fe9466299 100644
--- a/clang/include/clang/Lex/HeaderSearchOptions.h
+++ b/clang/include/clang/Lex/HeaderSearchOptions.h
@@ -219,6 +219,9 @@ class HeaderSearchOptions {
 
   unsigned ModulesValidateDiagnosticOptions : 1;
 
+  /// Whether to entirely skip writing diagnostic options.
+  unsigned ModulesSkipDiagnosticOptions : 1;
+
   unsigned ModulesHashContent : 1;
 
   /// Whether we should include all things that could impact the module in the
@@ -238,7 +241,8 @@ class HeaderSearchOptions {
 ModulesValidateSystemHeaders(false),
 ValidateASTInputFilesContent(false),
 ForceCheckCXX20ModulesInputFiles(false), UseDebugInfo(false),
-ModulesValidateDiagnosticOptions(true), ModulesHashContent(false),
+ModulesValidateDiagnosticOptions(true),
+ModulesSkipDiagnosticOptions(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 ab70594607530f4..c63f66f551b0eb6 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1215,6 +1215,9 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
   // Diagnostic options.
   const auto  = Context.getDiagnostics();
   const DiagnosticOptions  = Diags.getDiagnosticOptions();
+  if (!PP.getHeaderSearchInfo()
+   .getHeaderSearchOpts()
+   .ModulesSkipDiagnosticOptions) {
 #define DIAGOPT(Name, Bits, Default) Record.push_back(DiagOpts.Name);
 #define ENUM_DIAGOPT(Name, Type, Bits, Default)
\
   Record.push_back(static_cast(DiagOpts.get##Name()));
@@ -1229,6 +1232,7 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
   // are generally transient files and will almost always be overridden.
   Stream.EmitRecord(DIAGNOSTIC_OPTIONS, Record);
   Record.clear();
+  }
 
   // Header search paths.
   Record.clear();
diff --git a/clang/test/Modules/diagnostic-options-mismatch.c 
b/clang/test/Modules/diagnostic-options-mismatch.c
new file mode 100644
index 000..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

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-31 Thread Ben Langmuir via cfe-commits 

https://github.com/benlangmuir approved this pull request.


https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-31 Thread Jan Svoboda via cfe-commits 


@@ -1212,52 +1212,54 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
 Record.clear();
   }
 
+  const auto  = PP.getHeaderSearchInfo().getHeaderSearchOpts();
+
   // Diagnostic options.
   const auto  = Context.getDiagnostics();
   const DiagnosticOptions  = 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(DiagOpts.get##Name()));

jansvoboda11 wrote:

`clang-format` unfortunately does not indent this line.

https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-30 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 updated 
https://github.com/llvm/llvm-project/pull/69975

>From 0270c76e779457486ee89f100db2b7151832c290 Mon Sep 17 00:00:00 2001
From: Jan Svoboda 
Date: Tue, 10 Oct 2023 14:16:13 -0700
Subject: [PATCH 1/5] [clang][modules] Make `DIAGNOSTIC_OPTIONS` skippable

---
 clang/include/clang/Driver/Options.td |  4 ++
 clang/include/clang/Lex/HeaderSearchOptions.h |  6 ++-
 clang/lib/Serialization/ASTWriter.cpp |  4 ++
 .../Modules/diagnostic-options-mismatch.c | 41 +++
 4 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/Modules/diagnostic-options-mismatch.c

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>,
   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,
+NegFlag, 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..d6dd94fe9466299 100644
--- a/clang/include/clang/Lex/HeaderSearchOptions.h
+++ b/clang/include/clang/Lex/HeaderSearchOptions.h
@@ -219,6 +219,9 @@ class HeaderSearchOptions {
 
   unsigned ModulesValidateDiagnosticOptions : 1;
 
+  /// Whether to entirely skip writing diagnostic options.
+  unsigned ModulesSkipDiagnosticOptions : 1;
+
   unsigned ModulesHashContent : 1;
 
   /// Whether we should include all things that could impact the module in the
@@ -238,7 +241,8 @@ class HeaderSearchOptions {
 ModulesValidateSystemHeaders(false),
 ValidateASTInputFilesContent(false),
 ForceCheckCXX20ModulesInputFiles(false), UseDebugInfo(false),
-ModulesValidateDiagnosticOptions(true), ModulesHashContent(false),
+ModulesValidateDiagnosticOptions(true),
+ModulesSkipDiagnosticOptions(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..e063f8d6acc295a 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1215,6 +1215,9 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
   // Diagnostic options.
   const auto  = Context.getDiagnostics();
   const DiagnosticOptions  = Diags.getDiagnosticOptions();
+  if (!PP.getHeaderSearchInfo()
+   .getHeaderSearchOpts()
+   .ModulesSkipDiagnosticOptions) {
 #define DIAGOPT(Name, Bits, Default) Record.push_back(DiagOpts.Name);
 #define ENUM_DIAGOPT(Name, Type, Bits, Default)
\
   Record.push_back(static_cast(DiagOpts.get##Name()));
@@ -1229,6 +1232,7 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
   // are generally transient files and will almost always be overridden.
   Stream.EmitRecord(DIAGNOSTIC_OPTIONS, Record);
   Record.clear();
+  }
 
   // Header search paths.
   Record.clear();
diff --git a/clang/test/Modules/diagnostic-options-mismatch.c 
b/clang/test/Modules/diagnostic-options-mismatch.c
new file mode 100644
index 000..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

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-25 Thread Chuanqi Xu via cfe-commits 


@@ -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.

ChuanqiXu9 wrote:

Then let's try to add comments about these two options are primarily used for 
deps scanning. It is helpful for future developers.

https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-25 Thread Jan Svoboda via cfe-commits 

jansvoboda11 wrote:

> 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.

https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-25 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 updated 
https://github.com/llvm/llvm-project/pull/69975

>From 0270c76e779457486ee89f100db2b7151832c290 Mon Sep 17 00:00:00 2001
From: Jan Svoboda 
Date: Tue, 10 Oct 2023 14:16:13 -0700
Subject: [PATCH 1/4] [clang][modules] Make `DIAGNOSTIC_OPTIONS` skippable

---
 clang/include/clang/Driver/Options.td |  4 ++
 clang/include/clang/Lex/HeaderSearchOptions.h |  6 ++-
 clang/lib/Serialization/ASTWriter.cpp |  4 ++
 .../Modules/diagnostic-options-mismatch.c | 41 +++
 4 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/Modules/diagnostic-options-mismatch.c

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>,
   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,
+NegFlag, 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..d6dd94fe9466299 100644
--- a/clang/include/clang/Lex/HeaderSearchOptions.h
+++ b/clang/include/clang/Lex/HeaderSearchOptions.h
@@ -219,6 +219,9 @@ class HeaderSearchOptions {
 
   unsigned ModulesValidateDiagnosticOptions : 1;
 
+  /// Whether to entirely skip writing diagnostic options.
+  unsigned ModulesSkipDiagnosticOptions : 1;
+
   unsigned ModulesHashContent : 1;
 
   /// Whether we should include all things that could impact the module in the
@@ -238,7 +241,8 @@ class HeaderSearchOptions {
 ModulesValidateSystemHeaders(false),
 ValidateASTInputFilesContent(false),
 ForceCheckCXX20ModulesInputFiles(false), UseDebugInfo(false),
-ModulesValidateDiagnosticOptions(true), ModulesHashContent(false),
+ModulesValidateDiagnosticOptions(true),
+ModulesSkipDiagnosticOptions(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..e063f8d6acc295a 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1215,6 +1215,9 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
   // Diagnostic options.
   const auto  = Context.getDiagnostics();
   const DiagnosticOptions  = Diags.getDiagnosticOptions();
+  if (!PP.getHeaderSearchInfo()
+   .getHeaderSearchOpts()
+   .ModulesSkipDiagnosticOptions) {
 #define DIAGOPT(Name, Bits, Default) Record.push_back(DiagOpts.Name);
 #define ENUM_DIAGOPT(Name, Type, Bits, Default)
\
   Record.push_back(static_cast(DiagOpts.get##Name()));
@@ -1229,6 +1232,7 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
   // are generally transient files and will almost always be overridden.
   Stream.EmitRecord(DIAGNOSTIC_OPTIONS, Record);
   Record.clear();
+  }
 
   // Header search paths.
   Record.clear();
diff --git a/clang/test/Modules/diagnostic-options-mismatch.c 
b/clang/test/Modules/diagnostic-options-mismatch.c
new file mode 100644
index 000..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

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-23 Thread Chuanqi Xu via cfe-commits 

https://github.com/ChuanqiXu9 commented:

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.

https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-23 Thread Ben Langmuir via cfe-commits 


@@ -2944,6 +2944,10 @@ def fno_modules_validate_textual_header_includes :
   
MarshallingInfoNegativeFlag>,
   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",

benlangmuir wrote:

Seems worth having a test, and a (frontend) option seems like the easiest way 
to do that.

https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-23 Thread Jan Svoboda via cfe-commits 


@@ -2944,6 +2944,10 @@ def fno_modules_validate_textual_header_includes :
   
MarshallingInfoNegativeFlag>,
   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",

jansvoboda11 wrote:

I did cherry-pick `-fmodules-skip-diagnostic-options`, since that's needed 
downstream and I wanted to keep things aligned upstream. There's no such need 
for header search paths flag, but if we want to test the behavior, I'm happy to 
add that. WDYT?

https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-23 Thread Ben Langmuir via cfe-commits 


@@ -1212,9 +1212,12 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
 Record.clear();
   }
 
+  const auto  = PP.getHeaderSearchInfo().getHeaderSearchOpts();
+
   // Diagnostic options.
   const auto  = Context.getDiagnostics();
   const DiagnosticOptions  = Diags.getDiagnosticOptions();
+  if (!HSOpts.ModulesSkipDiagnosticOptions) {

benlangmuir wrote:

Now that we're doing this upstream we should re-indent this code.

https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-23 Thread Ben Langmuir via cfe-commits 


@@ -2944,6 +2944,10 @@ def fno_modules_validate_textual_header_includes :
   
MarshallingInfoNegativeFlag>,
   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",

benlangmuir wrote:

Should we do the same for HSOpts here so we can add a test for it?

https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-23 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 edited 
https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-23 Thread via cfe-commits 

llvmbot wrote:




@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
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>,
   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,
+NegFlag, 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 
,
 Record.clear();
   }
 
+  const auto  = PP.getHeaderSearchInfo().getHeaderSearchOpts();
+
   // Diagnostic options.
   const auto  = Context.getDiagnostics();
   const DiagnosticOptions  = 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(DiagOpts.get##Name()));
@@ -1229,35 +1232,34 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
   // are generally transient files and will almost always be overridden.
   Stream.EmitRecord(DIAGNOSTIC_OPTIONS, Record);
   Record.clear();
+  }
 
   // Header search paths.
-  Record.clear();
-  const HeaderSearchOptions  =
-  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  = HSOpts.UserEntries[I];
-AddString(Entry.Path, Record);
-Record.push_back(static_cast(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

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-23 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 edited 
https://github.com/llvm/llvm-project/pull/69975
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Skip slow `UNHASHED_CONTROL_BLOCK` records (PR #69975)

2023-10-23 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 created 
https://github.com/llvm/llvm-project/pull/69975

Deserialization of the `DIAGNOSTIC_OPTIONS` and `HEADER_SEARCH_PATHS` records 
is slow and done for every transitively loaded PCM. These records cannot be 
skipped, because the words are VBR6-encoded and we don't store the length. We 
could either make them binary blobs that can be skipped during 
deserialiazation, or skip writing them altogether. This patch takes the latter 
approach, since these records are not necessary in the dependency scanner. 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.


>From 0270c76e779457486ee89f100db2b7151832c290 Mon Sep 17 00:00:00 2001
From: Jan Svoboda 
Date: Tue, 10 Oct 2023 14:16:13 -0700
Subject: [PATCH 1/3] [clang][modules] Make `DIAGNOSTIC_OPTIONS` skippable

---
 clang/include/clang/Driver/Options.td |  4 ++
 clang/include/clang/Lex/HeaderSearchOptions.h |  6 ++-
 clang/lib/Serialization/ASTWriter.cpp |  4 ++
 .../Modules/diagnostic-options-mismatch.c | 41 +++
 4 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/Modules/diagnostic-options-mismatch.c

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>,
   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,
+NegFlag, 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..d6dd94fe9466299 100644
--- a/clang/include/clang/Lex/HeaderSearchOptions.h
+++ b/clang/include/clang/Lex/HeaderSearchOptions.h
@@ -219,6 +219,9 @@ class HeaderSearchOptions {
 
   unsigned ModulesValidateDiagnosticOptions : 1;
 
+  /// Whether to entirely skip writing diagnostic options.
+  unsigned ModulesSkipDiagnosticOptions : 1;
+
   unsigned ModulesHashContent : 1;
 
   /// Whether we should include all things that could impact the module in the
@@ -238,7 +241,8 @@ class HeaderSearchOptions {
 ModulesValidateSystemHeaders(false),
 ValidateASTInputFilesContent(false),
 ForceCheckCXX20ModulesInputFiles(false), UseDebugInfo(false),
-ModulesValidateDiagnosticOptions(true), ModulesHashContent(false),
+ModulesValidateDiagnosticOptions(true),
+ModulesSkipDiagnosticOptions(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..e063f8d6acc295a 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1215,6 +1215,9 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
   // Diagnostic options.
   const auto  = Context.getDiagnostics();
   const DiagnosticOptions  = Diags.getDiagnosticOptions();
+  if (!PP.getHeaderSearchInfo()
+   .getHeaderSearchOpts()
+   .ModulesSkipDiagnosticOptions) {
 #define DIAGOPT(Name, Bits, Default) Record.push_back(DiagOpts.Name);
 #define ENUM_DIAGOPT(Name, Type, Bits, Default)
\
   Record.push_back(static_cast(DiagOpts.get##Name()));
@@ -1229,6 +1232,7 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor 
,
   // are generally transient files and will almost always be overridden.
   Stream.EmitRecord(DIAGNOSTIC_OPTIONS, Record);
   Record.clear();
+  }
 
   // Header search paths.
   Record.clear();
diff --git a/clang/test/Modules/diagnostic-options-mismatch.c 
b/clang/test/Modules/diagnostic-options-mismatch.c
new file mode 100644
index 000..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