[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
ChuanqiXu9 wrote: Yeah, it'll be much better if we had that feature. https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
mathstuf wrote: > Sorry for that. But I think it may not be a big deal since people who still > want to take an eye this can made it by clicking two times simply. The timing is also unfortunate as some may have already gone on vacation and will miss updates until they return. Anyways, what's done is done. > The more the comments are, the more confused readers get. Generally a cleaner > PR is not suggested since it lost the context. It is unfortunate that Github sucks so much at rendering discussions (e.g., GItLab's threads that can be resolved makes it easy to collapse down finished things), There is the "mark as off-topic" strategy that I've seen used before, but that is also conflating the desire to hide things with spam control. In the future I would suggest mentioning those who have participated in the prior PR directly so that there's no gap in email/notification coverage. https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
ChuanqiXu9 wrote: > > Since there is no meaningful review opinions here, let's continue it in a > > new and clean PR: #75894 > > Note that this leaves behind all who have hit "Subscribe" on this PR to keep > tabs on things (I wish Github provided a way to mark-as-duplicate and merge > the subscriber list). All of the references to this PR are now confused as > well. What benefit does a "clean" PR provide? >From my experience, a cleaner PR will be easier for new people to get the >context or we look back after times. The more the comments are, the more >confused readers get. Generally a cleaner PR is not suggested since it lost >the context. But for this specific example, I think it makes sense since the >context here is not so meaning full. > Note that this leaves behind all who have hit "Subscribe" on this PR to keep > tabs on things Sorry for that. But I think it may not be a big deal since people who still want to take an eye this can made it by clicking two times simply. https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
mathstuf wrote: > Since there is no meaningful review opinions here, let's continue it in a new > and clean PR: https://github.com/llvm/llvm-project/pull/75894 Note that this leaves behind all who have hit "Subscribe" on this PR to keep tabs on things (I wish Github provided a way to mark-as-duplicate and merge the subscriber list). All of the references to this PR are now confused as well. What benefit does a "clean" PR provide? https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
https://github.com/ChuanqiXu9 closed https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
ChuanqiXu9 wrote: Since there is no meaningful review opinions here, let's continue it in a new and clean PR: https://github.com/llvm/llvm-project/pull/75894 https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
dwblaikie wrote: The words probably don't need to be short. Interface BMI Implementation BMI Seem like fine, accurate terms. I guess we could say "BMInterface" and "BMImplementation" but probably best to jus tgloss over "I" being "interface" and have "interface BMI" and "implementation BMI" https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
ChuanqiXu9 wrote: Let's discuss the higher level problems in https://discourse.llvm.org/t/rfc-c-20-modules-introduce-thin-bmi-and-decls-hash/74755 https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
ChuanqiXu9 wrote: The term `sparse` makes me wondering its format is not efficiency.. And for the term `surface`, ..., it may beyond my knowledge, I feel it is odd but I can't give a concrete reason. https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
tschuett wrote: It could also be a `surface BMI` as it only describes the surface of the module, but it does not contain its content. https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
iains wrote: perhaps in the context we can use "sparse" instead of "thin" - but I do not have a good short word replace 'fat'. https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
@@ -270,6 +274,35 @@ namespace clang {
};
}
+bool clang::MayDefAffectABI(const Decl *D) {
+ if (auto *FD = dyn_cast(D)) {
tbaederr wrote:
```suggestion
if (const auto *FD = dyn_cast(D)) {
```
https://github.com/llvm/llvm-project/pull/71622
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
@@ -270,6 +274,35 @@ namespace clang {
};
}
+bool clang::MayDefAffectABI(const Decl *D) {
+ if (auto *FD = dyn_cast(D)) {
+if (FD->isInlined() || FD->isConstexpr())
+ return true;
+
+// Non-user-provided functions get emitted as weak definitions with every
+// use, no matter whether they've been explicitly instantiated etc.
+if (!FD->isUserProvided())
+ return true;
+
+if (FD->isDependentContext())
+ return true;
+
+if (FD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation)
+ return true;
+ }
+
+ if (auto *VD = dyn_cast(D)) {
tbaederr wrote:
```suggestion
if (const auto *VD = dyn_cast(D)) {
```
https://github.com/llvm/llvm-project/pull/71622
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
ChuanqiXu9 wrote: > It was suggested to avoid "thin" and "fat" terminology (I started using it > with LTO as a precedent for the terms), but I suppose something more > descriptive could be selected. Any suggestions? My english is not good, you know. > I'll also note that my prior usage was in reference to embedding transitive > BMI references in the BMI, not about including function definitions. May you elaborate on this? I don't understand since I feel they are unrelated. Are you saying: https://github.com/llvm/llvm-project/issues/62707? I feel they are orthogonal. https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
mathstuf wrote: I'll also note that my prior usage was in reference to embedding transitive BMI references in the BMI, not about including function definitions. https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
mathstuf wrote: It was suggested to avoid "thin" and "fat" terminology (I started using it with LTO as a precedent for the terms), but I suppose something more descriptive could be selected. https://github.com/llvm/llvm-project/pull/71622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff b85fdc4ffde48f2035e4621d4f9af69bf16f2136
839d38d96d2275dbc0a9aa9fae3427720a6d3344 --
clang/include/clang/Frontend/FrontendActions.h
clang/include/clang/Frontend/FrontendOptions.h
clang/include/clang/Serialization/ASTWriter.h clang/lib/Driver/Driver.cpp
clang/lib/Driver/ToolChains/Clang.cpp clang/lib/Frontend/CompilerInvocation.cpp
clang/lib/Frontend/FrontendActions.cpp
clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
clang/lib/Serialization/ASTWriter.cpp clang/lib/Serialization/ASTWriterDecl.cpp
clang/lib/Serialization/GeneratePCH.cpp
clang/test/CXX/basic/basic.link/p10-ex2.cpp
clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p4-friend-in-reachable-class.cpp
clang/test/Modules/Reachability-Private.cpp
clang/test/Modules/Reachability-func-default-arg.cpp
clang/test/Modules/Reachability-func-ret.cpp
clang/test/Modules/Reachability-template-default-arg.cpp
clang/test/Modules/Reachability-template-instantiation.cpp
clang/test/Modules/Reachability-using-templates.cpp
clang/test/Modules/Reachability-using.cpp clang/test/Modules/cxx20-10-1-ex1.cpp
clang/test/Modules/cxx20-10-1-ex2.cpp clang/test/Modules/cxx20-10-2-ex2.cpp
clang/test/Modules/cxx20-10-2-ex5.cpp clang/test/Modules/cxx20-10-3-ex1.cpp
clang/test/Modules/cxx20-10-3-ex2.cpp clang/test/Modules/cxx20-10-5-ex1.cpp
clang/test/Modules/cxx20-import-diagnostics-a.cpp
clang/test/Modules/cxx20-import-diagnostics-b.cpp
clang/test/Modules/cxx20-module-file-info-macros.cpp
clang/test/Modules/derived_class.cpp
clang/test/Modules/explicitly-specialized-template.cpp
clang/test/Modules/merge-concepts-cxx-modules.cpp
clang/test/Modules/merge-constrained-friends.cpp
clang/test/Modules/mismatch-diagnostics.cpp
clang/test/Modules/placement-new-reachable.cpp
clang/test/Modules/predefined.cpp
clang/test/Modules/redundant-template-default-arg.cpp
clang/test/Modules/redundant-template-default-arg2.cpp
clang/test/Modules/redundant-template-default-arg3.cpp
clang/test/Modules/search-partitions.cpp
clang/test/Modules/template-function-specialization.cpp
clang/test/Modules/template_default_argument.cpp
clang/unittests/Sema/SemaNoloadLookupTest.cpp
clang/unittests/Serialization/ForceCheckFileInputTest.cpp
clang/unittests/Serialization/NoCommentsTest.cpp
clang/unittests/Serialization/VarDeclConstantInitTest.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp
b/clang/lib/Serialization/ASTWriterDecl.cpp
index 00a04ee2fa73..ca7794b8c5f9 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -283,7 +283,7 @@ bool clang::MayDefAffectABI(const Decl *D) {
// use, no matter whether they've been explicitly instantiated etc.
if (!FD->isUserProvided())
return true;
-
+
if (FD->isDependentContext())
return true;
``
https://github.com/llvm/llvm-project/pull/71622
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20] [Modules] Introduce thin BMI (PR #71622)
llvmbot wrote: @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) Changes Close https://github.com/llvm/llvm-project/issues/71034 This patch introduces thin BMI, which doesn't contain the definitions of functions and variables if its definitions won't contribute to the ABI. Testing is a big part of the patch. We want to make sure the thin BMI contains the same behavior with the existing and relatively stable fatBMI. This is pretty helpful for further reduction. For user interfeaces, this patch introduces `-fthinBMI-output=` arguments to specify the position of thin BMI. This should be used when compiling a single module unit. The design is helpful to use thin BMI in two phase compilations too. With thin BMI, In two phase compilations, we'll generate 2 BMIs, one thin BMI for being used by consumers, one fat BMI for compiling itself to object files. Maybe it sounds confusing to have 2 BMIs for one module unit. But only the thin BMI will be the BMI we're talking about generally and the fat BMI is only visible by the module unit itself. With one phase compilation, we may find the behavior of `-fthinBMI-output=` is pretty similar with `-fmodule-output=`, except one generating thin BMI and the other generating fat BMI. The design here is based on 2 things: (1) The serialization of C++ is pretty complex. We can't be sure we're handling every detail correctly in the every beginning. (2) The fat BMI is relatively widely used and relatively stable. So it looks not good to replace the fat BMI immediately with thin BMI. But, of course, in the end of the day, we want the consumers to use the thin BMI only. When that day comes, the `-fmodule-output=` will be an alias to `-fthinBMI-output=`. Another design choice is to reuse `-fmodule-output=` and introduce a flag `-femit-thin-BMI`. Then `-femit-thin-BMI -fmodule-output=` will have the same effect with `-fthinBMI-output=` now. The flag `-femit-thin-BMI` should be opt-in now and opt-off later and finally deprecated. The roadmap for thin BMI in my mind is: (1) In clang18, release thin BMI and mark it as experimental. Also encourage users and build systems to try this new mode. (2) In clang19 or clang20 (based on the issue feedbacks), remove the experimental mark for thin BMI and mark fat BMI as deprecated to be used by consumers. (3) In clang21 or clang22, error out if we found the users are trying to import a fat BMI. CC: @mathstuf --- Patch is 113.03 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/71622.diff 112 Files Affected: - (modified) clang/include/clang/Basic/DiagnosticDriverKinds.td (+2) - (modified) clang/include/clang/Driver/Options.td (+8-1) - (modified) clang/include/clang/Frontend/FrontendActions.h (+17-1) - (modified) clang/include/clang/Frontend/FrontendOptions.h (+8-1) - (modified) clang/include/clang/Serialization/ASTWriter.h (+29-2) - (modified) clang/lib/Driver/Driver.cpp (+7) - (modified) clang/lib/Driver/ToolChains/Clang.cpp (+2) - (modified) clang/lib/Frontend/CompilerInvocation.cpp (+2) - (modified) clang/lib/Frontend/FrontendActions.cpp (+47-5) - (modified) clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp (+2) - (modified) clang/lib/Serialization/ASTWriter.cpp (+18-14) - (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+46-7) - (modified) clang/lib/Serialization/GeneratePCH.cpp (+34-2) - (modified) clang/test/CXX/basic/basic.link/p10-ex2.cpp (+2-1) - (modified) clang/test/CXX/basic/basic.lookup/basic.lookup.argdep/p4-friend-in-reachable-class.cpp (+4-2) - (added) clang/test/Driver/thinBMI-output.cppm (+29) - (modified) clang/test/Modules/InheritDefaultArguments.cppm (+3) - (modified) clang/test/Modules/Reachability-Private.cpp (+10) - (modified) clang/test/Modules/Reachability-func-default-arg.cpp (+3) - (modified) clang/test/Modules/Reachability-func-ret.cpp (+3) - (modified) clang/test/Modules/Reachability-template-default-arg.cpp (+3) - (modified) clang/test/Modules/Reachability-template-instantiation.cpp (+4) - (modified) clang/test/Modules/Reachability-using-templates.cpp (+3) - (modified) clang/test/Modules/Reachability-using.cpp (+3) - (modified) clang/test/Modules/concept.cppm (+4) - (modified) clang/test/Modules/concept_differ.cppm (+5) - (modified) clang/test/Modules/ctor.arg.dep.cppm (+4) - (modified) clang/test/Modules/cxx20-10-1-ex1.cpp (+13) - (modified) clang/test/Modules/cxx20-10-1-ex2.cpp (+30-6) - (modified) clang/test/Modules/cxx20-10-2-ex2.cpp (+12) - (modified) clang/test/Modules/cxx20-10-2-ex5.cpp (+12) - (modified) clang/test/Modules/cxx20-10-3-ex1.cpp (+14) - (modified) clang/test/Modules/cxx20-10-3-ex2.cpp (+10) - (modified) clang/test/Modules/cxx20-10-5-ex1.cpp (+12) - (modified) clang/test/Modules/cxx20-import-diagnostics-a.cpp (+39) - (modified) clang/test/Modules/cxx20-import-diagnostics-b.cpp (+25) - (modified) clang/test/Modules/cxx20-module-file-info-macros.cpp
