[clang] [C++20] [Modules] Introduce reduced BMI (PR #75894)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> Can we add a release note and documentation for this? Thanks!

The current patch is transparent to users and it is only part of the series 
patches. I'd like to document that after I made the series of patches.

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

[clang] d3df2a8 - [C++20] [Modules] Handle transitive import in the module properly

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-03-06T15:46:55+08:00
New Revision: d3df2a834cf6febb44c699d109b9e7f622194837

URL: 
https://github.com/llvm/llvm-project/commit/d3df2a834cf6febb44c699d109b9e7f622194837
DIFF: 
https://github.com/llvm/llvm-project/commit/d3df2a834cf6febb44c699d109b9e7f622194837.diff

LOG: [C++20] [Modules] Handle transitive import in the module properly

Close https://github.com/llvm/llvm-project/issues/84002

Per [module.import]p7:

> Additionally, when a module-import-declaration in a module unit of
> some module M imports another module unit U of M, it also imports all
> translation units imported by non-exported module-import-declarations
> in the module unit purview of U.

However, we only tried to implement it during the implicit import of
primary module interface for module implementation unit.

Also we didn't implement the last sentence from [module.import]p7
completely:

> These rules can in turn lead to the importation of yet more
> translation units.

This patch tries to care the both issues.

Added: 
clang/test/Modules/transitive-import.cppm

Modified: 
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/Module.h
clang/lib/Basic/Module.cpp
clang/lib/Sema/SemaModule.cpp

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5e0352a7eaf6cd..b074055a4eaf69 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -86,6 +86,11 @@ C++20 Feature Support
 - Implemented the `__is_layout_compatible` intrinsic to support
   `P0466R5: Layout-compatibility and Pointer-interconvertibility Traits 
`_.
 
+- Clang now implements [module.import]p7 fully. Clang now will import module
+  units transitively for the module units coming from the same module of the
+  current module units.
+  Fixes `#84002 `_.
+
 C++23 Feature Support
 ^
 

diff  --git a/clang/include/clang/Basic/Module.h 
b/clang/include/clang/Basic/Module.h
index 30ec9c99315092..9f62c058ca0da0 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -598,6 +598,11 @@ class alignas(8) Module {
Kind == ModulePartitionImplementation;
   }
 
+  /// Is this a module partition implementation unit.
+  bool isModulePartitionImplementation() const {
+return Kind == ModulePartitionImplementation;
+  }
+
   /// Is this a module implementation.
   bool isModuleImplementation() const {
 return Kind == ModuleImplementationUnit;
@@ -853,12 +858,6 @@ class VisibleModuleSet {
   VisibleCallback Vis = [](Module *) {},
   ConflictCallback Cb = [](ArrayRef, Module *,
StringRef) {});
-
-  /// Make transitive imports visible for [module.import]/7.
-  void makeTransitiveImportsVisible(
-  Module *M, SourceLocation Loc, VisibleCallback Vis = [](Module *) {},
-  ConflictCallback Cb = [](ArrayRef, Module *, StringRef) {});
-
 private:
   /// Import locations for each visible module. Indexed by the module's
   /// VisibilityID.

diff  --git a/clang/lib/Basic/Module.cpp b/clang/lib/Basic/Module.cpp
index 1c5043a618fff3..9f597dcf8b0f5a 100644
--- a/clang/lib/Basic/Module.cpp
+++ b/clang/lib/Basic/Module.cpp
@@ -722,14 +722,6 @@ void VisibleModuleSet::setVisible(Module *M, 
SourceLocation Loc,
   VisitModule({M, nullptr});
 }
 
-void VisibleModuleSet::makeTransitiveImportsVisible(Module *M,
-SourceLocation Loc,
-VisibleCallback Vis,
-ConflictCallback Cb) {
-  for (auto *I : M->Imports)
-setVisible(I, Loc, Vis, Cb);
-}
-
 ASTSourceDescriptor::ASTSourceDescriptor(Module &M)
 : Signature(M.Signature), ClangModule(&M) {
   if (M.Directory)

diff  --git a/clang/lib/Sema/SemaModule.cpp b/clang/lib/Sema/SemaModule.cpp
index ed7f626971f345..f08c1cb3a13ef5 100644
--- a/clang/lib/Sema/SemaModule.cpp
+++ b/clang/lib/Sema/SemaModule.cpp
@@ -73,6 +73,90 @@ static std::string stringFromPath(ModuleIdPath Path) {
   return Name;
 }
 
+/// Helper function for makeTransitiveImportsVisible to decide whether
+/// the \param Imported module unit is in the same module with the \param
+/// CurrentModule.
+/// \param FoundPrimaryModuleInterface is a helper parameter to record the
+/// primary module interface unit corresponding to the module \param
+/// CurrentModule. Since currently it is expensive to decide whether two module
+/// units come from the same module by comparing the module name.
+static bool
+isImportingModuleUnitFromSameModule(Module *Imported, Module *CurrentModule,
+Module *&FoundPrimaryModuleInterface) {
+  if (!Imported->isNamedModule())
+return false;
+
+  // The a partition 

[clang] [C++20] [Modules] Introduce reduced BMI (PR #75894)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

I am going to land this in the week later if no objections come in. I think it 
is necessary to land the series of patches (to reduce the contents of BMI) for 
clang19. And of course, the functionality will be opt in for one~two releases 
for experimental.

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

[clang] [C++20] [Modules] Introduce reduced BMI (PR #75894)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> Do you expect to make any changes to type streaming?

I don't expect to do that explicitly. The number of types deserialized can be 
decreased naturally after we avoid emitting declarations during the writing.

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

[clang] [C++20] [Modules] Introduce reduced BMI (PR #75894)

[Chuanqi Xu via cfe-commits]

@@ -830,6 +843,19 @@ class PCHGenerator : public SemaConsumer {
   bool hasEmittedPCH() const { return Buffer->IsComplete; }
 };
 
+class ReducedBMIGenerator : public PCHGenerator {
+public:
+  ReducedBMIGenerator(const Preprocessor &PP, InMemoryModuleCache &ModuleCache,
+  StringRef OutputFile, std::shared_ptr Buffer,
+  bool IncludeTimestamps);
+
+  void HandleTranslationUnit(ASTContext &Ctx) override;
+};
+
+/// If the definition may impact the ABI. If yes, we're allowed to eliminate
+/// the definition of D in reduced BMI.
+bool MayDefAffectABI(const Decl *D);

ChuanqiXu9 wrote:

Nice catch up.

I changed the name to `CanElideDeclDef` and the comments to:

```
/// If we can elide the definition of \param D in reduced BMI.
///
/// Generally, we can elide the definition of a declaration if it won't affect 
the
/// ABI. e.g., the non-inline function bodies.
```

Hope this to be clear.

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

[clang] [C++20] [Modules] Introduce reduced BMI (PR #75894)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> * I do not want to block progress, so let's move forward with this patch for 
> now.

Yeah. Great to see we have some progress finally. I think this is really 
important since I see more and more peope complaninig the performance for 
modules. I feel this series patch is key to to improve the user's impression 
that named modules are just purely wrappers for PCH.

> * It seems to me (as we found with GMF decl elision) that the process is 
> quite a bit more complex than simply omitting a decl.  We need to elide other 
> decls that are then unused (e.g. decls local to an elided function body) and 
> also avoid emitting types that are now no longer existent.

After looking into the codes more, I changed my mind for this. I feel it is the 
most efficient and the most natural way to omit declarations in ASTWriter.

The idea is, previously, we'll start to write declarations in the current TU 
from the first declarations. And in the C++20 named modules world, we would 
start to write declarations from the first declarations in the global module 
fragment.  And we can implement GMF decl elision naturally by writing 
declarations from the first declaration in the module purview and only write 
the referenced decl from the GMF during the writing process. This is super 
natural and efficient.

> 
> The process seems to me to be either one of:
> 
> * rewriting the AST (which is why my patch set picked the use of the plugin 
> API since that is the purpose there).
> * walking the AST and marking entities as used / not used / elided.

Now I doubt both of the method to be not efficiency and it adds additional 
burdens to the developers and the users.

> 
> It still feels to me to be better to have clear separation of this work from 
> the work of streaming - but if we can make clear layers within the streaming, 
> then maybe the maintenance will not be too hard.

I think the maintainance may not be too hard in that way. ASTWriter is not such 
a devil :) Probably we need some refactoration in the serialization to make 
codes more clear. But the point is that we must get the ASTWriter/ASTReader 
involved. It may not be a good idea to leave the ASTWriter/ASTReader as is and 
add another layer... 

For example, it should be better to not deserialize the declaration from the 
very beginning instead of deserializing the declaration and judge that it is 
not wanted/visible. And to achieve this, we must  touch the serialization layer 
deeply.



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

[clang] [clang][NFC] Format clang/lib/Sema/Sema.cpp (PR #83974)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> Thank you for references both.
> 
> Actually, I'd like to have PR like #83961. Is it acceptable to merge this 
> kind of PR in that case then?

I don't feel the patches are related. I think you can only format the changed 
lines.

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

[clang] [clang][NFC] Format clang/lib/Sema/Sema.cpp (PR #83974)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

Oh, I found it here: https://llvm.org/docs/CodingStandards.html#introduction

> Our long term goal is for the entire codebase to follow the convention, but 
> we explicitly do not want patches that do large-scale reformatting of 
> existing code.

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

[clang] [clang][NFC] Format clang/lib/Sema/Sema.cpp (PR #83974)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

I remember we have policies that we don't like patches which purely formats 
codes. It makes backporting and cherry-picking harder. But I can't find the 
wording now.

CC: @AaronBallman @Endilll 

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

Thanks. This looks good to me except few nit comments.

Have you tested on a real world workloads?

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -167,6 +167,53 @@ class CoroCloner {
 
 } // end anonymous namespace
 
+// FIXME:
+// Lower the intrinisc in CoroEarly phase if coroutine frame doesn't escape
+// and it is known that other transformations, for example, sanitizers
+// won't lead to incorrect code.
+static void lowerAwaitSuspend(IRBuilder<> &Builder, CoroAwaitSuspendInst *CB) {
+  auto Wrapper = CB->getWrapperFunction();
+  auto Awaiter = CB->getAwaiter();
+  auto FramePtr = CB->getFrame();
+
+  Builder.SetInsertPoint(CB);
+
+  CallBase *NewCall = nullptr;
+  if (auto Invoke = dyn_cast(CB)) {
+auto WrapperInvoke =
+Builder.CreateInvoke(Wrapper, Invoke->getNormalDest(),
+ Invoke->getUnwindDest(), {Awaiter, FramePtr});
+
+WrapperInvoke->setCallingConv(Invoke->getCallingConv());
+std::copy(Invoke->bundle_op_info_begin(), Invoke->bundle_op_info_end(),
+  WrapperInvoke->bundle_op_info_begin());
+AttributeList NewAttributes =
+Invoke->getAttributes().removeParamAttributes(Invoke->getContext(), 2);

ChuanqiXu9 wrote:

What is the attribute to be removed? Let's add a comment here to make it clear.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -338,6 +414,71 @@ static QualType getCoroutineSuspendExprReturnType(const 
ASTContext &Ctx,
 }
 #endif
 
+llvm::Function *
+CodeGenFunction::generateAwaitSuspendWrapper(Twine const &CoroName,
+ Twine const &SuspendPointName,
+ CoroutineSuspendExpr const &S) {
+  std::string FuncName = "__await_suspend_wrapper_";
+  FuncName += CoroName.str();
+  FuncName += '_';
+  FuncName += SuspendPointName.str();
+
+  ASTContext &C = getContext();
+
+  FunctionArgList args;
+
+  ImplicitParamDecl AwaiterDecl(C, C.VoidPtrTy, ImplicitParamKind::Other);
+  ImplicitParamDecl FrameDecl(C, C.VoidPtrTy, ImplicitParamKind::Other);
+  QualType ReturnTy = S.getSuspendExpr()->getType();
+
+  args.push_back(&AwaiterDecl);
+  args.push_back(&FrameDecl);
+
+  const CGFunctionInfo &FI =
+  CGM.getTypes().arrangeBuiltinFunctionDeclaration(ReturnTy, args);
+
+  llvm::FunctionType *LTy = CGM.getTypes().GetFunctionType(FI);
+
+  llvm::Function *Fn = llvm::Function::Create(
+  LTy, llvm::GlobalValue::PrivateLinkage, FuncName, &CGM.getModule());
+
+  Fn->addParamAttr(0, llvm::Attribute::AttrKind::NonNull);
+  Fn->addParamAttr(0, llvm::Attribute::AttrKind::NoUndef);
+
+  Fn->addParamAttr(1, llvm::Attribute::AttrKind::NoUndef);
+
+  Fn->setMustProgress();
+  Fn->addFnAttr(llvm::Attribute::AttrKind::AlwaysInline);
+
+  StartFunction(GlobalDecl(), ReturnTy, Fn, FI, args);
+
+  llvm::Value *AwaiterPtr = 
Builder.CreateLoad(GetAddrOfLocalVar(&AwaiterDecl));
+  auto AwaiterLValue =
+  MakeNaturalAlignAddrLValue(AwaiterPtr, AwaiterDecl.getType());
+
+  CurAwaitSuspendWrapper.FramePtr =
+  Builder.CreateLoad(GetAddrOfLocalVar(&FrameDecl));
+
+  // FIXME: mark as aliasing with frame?
+  // FIXME: TBAA?
+  // FIXME: emit in a better way (maybe egenerate AST in SemaCoroutine)?

ChuanqiXu9 wrote:

Generally we like comments in a more formal style. Please rewrite the comments 
to make it more expressive.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

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

[clang] [clang-tools-extra] [libcxx] [clang] Enable sized deallocation by default in C++14 onwards (PR #83774)

[Chuanqi Xu via cfe-commits]

@@ -72,6 +72,10 @@ sections with improvements to Clang's support for those 
languages.
 C++ Language Changes
 
 
+C++14 Feature Support
+^
+- Sized deallocation is enabled by default in C++14 onwards.

ChuanqiXu9 wrote:

It may be good to mention the flag enabled so that people met regression can 
disable it locally to workaround it.

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

[clang] [clang-tools-extra] [libcxx] [clang] Enable sized deallocation by default in C++14 onwards (PR #83774)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,44 @@
+//===--- SizedDellocation.h - Sized Deallocation *- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// Defines a function that returns the minimum OS versions supporting
+/// C++14's sized deallocation functions.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_BASIC_SIZEDDEALLOCATION_H
+#define LLVM_CLANG_BASIC_SIZEDDEALLOCATION_H
+
+#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/VersionTuple.h"
+#include "llvm/TargetParser/Triple.h"
+
+namespace clang {
+inline llvm::VersionTuple sizedDeallocMinVersion(llvm::Triple::OSType OS) {

ChuanqiXu9 wrote:

If I read correctly, this is only used in `Darwin.cpp`, so it would be better 
to inline the function there.

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

[clang] Reland "[clang][modules] Print library module manifest path." (PR #82160)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

@mordante if we want this for 18, we need to land and backport it in this week.

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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> That'd mean that we "just" need to replace LazySpecializationInfo 
> *LazySpecializations = nullptr; with the on-disk hash table approach. That 
> would probably require centralizing that logic somewhere in the ASTReader 
> (the way this PR does) but with minimal changes wrt D41416.

@vgvassilev Let me try to double check your advice. In you suggestion, you 
suggest to replace `LazySpecializationInfo *LazySpecializations` with an 
on-disk hash map from an integer (hash value for template args) to  
LazySpecializationInfo in D41416 instead of another integer (DeclID, just like 
my patch)?

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

[clang] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one. (PR #83108)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/83108

>From 59e1880df74434e3c446705788d92b5949d99536 Mon Sep 17 00:00:00 2001
From: Vassil Vassilev 
Date: Sun, 7 Jan 2018 15:16:11 +0200
Subject: [PATCH 1/3] D41416: [modules] [pch] Do not deserialize all lazy
 template specializations when looking for one.

---
 clang/include/clang/AST/DeclTemplate.h| 36 -
 clang/lib/AST/DeclTemplate.cpp| 96 +--
 clang/lib/AST/ODRHash.cpp | 15 
 clang/lib/Serialization/ASTReader.cpp | 25 --
 clang/lib/Serialization/ASTReaderDecl.cpp | 36 ++---
 clang/lib/Serialization/ASTWriter.cpp | 21 -
 clang/lib/Serialization/ASTWriterDecl.cpp | 74 ++---
 7 files changed, 242 insertions(+), 61 deletions(-)

diff --git a/clang/include/clang/AST/DeclTemplate.h 
b/clang/include/clang/AST/DeclTemplate.h
index e3b6a7efb1127a..4ed9b58d4ff609 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -256,6 +256,9 @@ class TemplateArgumentList final
   TemplateArgumentList(const TemplateArgumentList &) = delete;
   TemplateArgumentList &operator=(const TemplateArgumentList &) = delete;
 
+  /// Create hash for the given arguments.
+  static unsigned ComputeODRHash(ArrayRef Args);
+
   /// Create a new template argument list that copies the given set of
   /// template arguments.
   static TemplateArgumentList *CreateCopy(ASTContext &Context,
@@ -730,6 +733,26 @@ class RedeclarableTemplateDecl : public TemplateDecl,
   }
 
   void anchor() override;
+  struct LazySpecializationInfo {
+uint32_t DeclID = ~0U;
+unsigned ODRHash = ~0U;
+bool IsPartial = false;
+LazySpecializationInfo(uint32_t ID, unsigned Hash = ~0U,
+   bool Partial = false)
+  : DeclID(ID), ODRHash(Hash), IsPartial(Partial) { }
+LazySpecializationInfo() { }
+bool operator<(const LazySpecializationInfo &Other) const {
+  return DeclID < Other.DeclID;
+}
+bool operator==(const LazySpecializationInfo &Other) const {
+  assert((DeclID != Other.DeclID || ODRHash == Other.ODRHash) &&
+ "Hashes differ!");
+  assert((DeclID != Other.DeclID || IsPartial == Other.IsPartial) &&
+ "Both must be the same kinds!");
+  return DeclID == Other.DeclID;
+}
+  };
+
 protected:
   template  struct SpecEntryTraits {
 using DeclType = EntryType;
@@ -770,7 +793,12 @@ class RedeclarableTemplateDecl : public TemplateDecl,
 return SpecIterator(isEnd ? Specs.end() : Specs.begin());
   }
 
-  void loadLazySpecializationsImpl() const;
+  void loadLazySpecializationsImpl(bool OnlyPartial = false) const;
+
+  void loadLazySpecializationsImpl(llvm::ArrayRef Args,
+   TemplateParameterList *TPL = nullptr) const;
+
+  Decl *loadLazySpecializationImpl(LazySpecializationInfo &LazySpecInfo) const;
 
   template 
   typename SpecEntryTraits::DeclType*
@@ -797,7 +825,7 @@ class RedeclarableTemplateDecl : public TemplateDecl,
 ///
 /// The first value in the array is the number of specializations/partial
 /// specializations that follow.
-uint32_t *LazySpecializations = nullptr;
+LazySpecializationInfo *LazySpecializations = nullptr;
 
 /// The set of "injected" template arguments used within this
 /// template.
@@ -2268,7 +2296,7 @@ class ClassTemplateDecl : public RedeclarableTemplateDecl 
{
   friend class TemplateDeclInstantiator;
 
   /// Load any lazily-loaded specializations from the external source.
-  void LoadLazySpecializations() const;
+  void LoadLazySpecializations(bool OnlyPartial = false) const;
 
   /// Get the underlying class declarations of the template.
   CXXRecordDecl *getTemplatedDecl() const {
@@ -3039,7 +3067,7 @@ class VarTemplateDecl : public RedeclarableTemplateDecl {
   friend class ASTDeclWriter;
 
   /// Load any lazily-loaded specializations from the external source.
-  void LoadLazySpecializations() const;
+  void LoadLazySpecializations(bool OnlyPartial = false) const;
 
   /// Get the underlying variable declarations of the template.
   VarDecl *getTemplatedDecl() const {
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index 3c217d6a6a5ae3..1babe39ee2a7e5 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -20,6 +20,8 @@
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
+#include "clang/AST/ODRHash.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/LLVM.h"
@@ -331,16 +333,43 @@ RedeclarableTemplateDecl::CommonBase 
*RedeclarableTemplateDecl::getCommonPtr() c
   return Common;
 }
 
-void RedeclarableTemplateDecl::loadLazySpecializationsImpl() const {
+void RedeclarableTemplateDecl::loadLazySpecializationsImpl(
+ bool Onl

[clang] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one. (PR #83108)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

Weird. I only see two failures in my local environment:

```
Failed Tests (2):
  Clang :: Modules/cxx-templates.cpp
  Clang :: Modules/odr_hash.cpp
```

And I saw both of them in my patch. It is simply order mismatches.

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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

Oh, I didn't notice you've removed D153003 already. But the branch name looks 
not good. So I've created a pr in 
https://github.com/llvm/llvm-project/pull/83108

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

[clang] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one. (PR #83108)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

Personally I feel this patch is good and the testing result from our workload 
shows it is good too. But it looks like the performance testing results from 
google @zygoloid @ilya-biryukov is not good. So maybe we need to wait for 
landing this. (It will be great if @ilya-biryukov would like to test again)

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

[clang] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one. (PR #83108)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 created 
https://github.com/llvm/llvm-project/pull/83108

This from https://reviews.llvm.org/D41416. And we plan to introduce on disk 
hash table based on this. See https://github.com/llvm/llvm-project/pull/76774.

Following off are cited from https://reviews.llvm.org/D41416:

Currently, we load all lazy template specializations when we search whether 
there is a suitable template specialization for a template. This is especially 
suboptimal with modules. If module B specializes a template from module A the 
ASTReader would only read the specialization DeclID. This is observed to be 
especially pathological when module B is stl. However, the template 
instantiator (almost immediately after) will call findSpecialization. In turn, 
findSpecialization will load all lazy specializations to give an answer.

This patch teaches findSpecialization to work with lazy specializations without 
having to deserialize their full content. It provides along with the DeclID an 
cross-TU stable ODRHash of the template arguments which is enough to decide if 
we have already specialization and which are the exact ones (we load all decls 
with the same hash to avoid potential collisions) to deserialize.

While we make finding a template specialization more memory-efficient we are 
far from being done. There are still a few places which trigger eager 
deserialization of template specializations: the places where we require 
completion of the redeclaration chain.



>From 59e1880df74434e3c446705788d92b5949d99536 Mon Sep 17 00:00:00 2001
From: Vassil Vassilev 
Date: Sun, 7 Jan 2018 15:16:11 +0200
Subject: [PATCH] D41416: [modules] [pch] Do not deserialize all lazy template
 specializations when looking for one.

---
 clang/include/clang/AST/DeclTemplate.h| 36 -
 clang/lib/AST/DeclTemplate.cpp| 96 +--
 clang/lib/AST/ODRHash.cpp | 15 
 clang/lib/Serialization/ASTReader.cpp | 25 --
 clang/lib/Serialization/ASTReaderDecl.cpp | 36 ++---
 clang/lib/Serialization/ASTWriter.cpp | 21 -
 clang/lib/Serialization/ASTWriterDecl.cpp | 74 ++---
 7 files changed, 242 insertions(+), 61 deletions(-)

diff --git a/clang/include/clang/AST/DeclTemplate.h 
b/clang/include/clang/AST/DeclTemplate.h
index e3b6a7efb1127a..4ed9b58d4ff609 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -256,6 +256,9 @@ class TemplateArgumentList final
   TemplateArgumentList(const TemplateArgumentList &) = delete;
   TemplateArgumentList &operator=(const TemplateArgumentList &) = delete;
 
+  /// Create hash for the given arguments.
+  static unsigned ComputeODRHash(ArrayRef Args);
+
   /// Create a new template argument list that copies the given set of
   /// template arguments.
   static TemplateArgumentList *CreateCopy(ASTContext &Context,
@@ -730,6 +733,26 @@ class RedeclarableTemplateDecl : public TemplateDecl,
   }
 
   void anchor() override;
+  struct LazySpecializationInfo {
+uint32_t DeclID = ~0U;
+unsigned ODRHash = ~0U;
+bool IsPartial = false;
+LazySpecializationInfo(uint32_t ID, unsigned Hash = ~0U,
+   bool Partial = false)
+  : DeclID(ID), ODRHash(Hash), IsPartial(Partial) { }
+LazySpecializationInfo() { }
+bool operator<(const LazySpecializationInfo &Other) const {
+  return DeclID < Other.DeclID;
+}
+bool operator==(const LazySpecializationInfo &Other) const {
+  assert((DeclID != Other.DeclID || ODRHash == Other.ODRHash) &&
+ "Hashes differ!");
+  assert((DeclID != Other.DeclID || IsPartial == Other.IsPartial) &&
+ "Both must be the same kinds!");
+  return DeclID == Other.DeclID;
+}
+  };
+
 protected:
   template  struct SpecEntryTraits {
 using DeclType = EntryType;
@@ -770,7 +793,12 @@ class RedeclarableTemplateDecl : public TemplateDecl,
 return SpecIterator(isEnd ? Specs.end() : Specs.begin());
   }
 
-  void loadLazySpecializationsImpl() const;
+  void loadLazySpecializationsImpl(bool OnlyPartial = false) const;
+
+  void loadLazySpecializationsImpl(llvm::ArrayRef Args,
+   TemplateParameterList *TPL = nullptr) const;
+
+  Decl *loadLazySpecializationImpl(LazySpecializationInfo &LazySpecInfo) const;
 
   template 
   typename SpecEntryTraits::DeclType*
@@ -797,7 +825,7 @@ class RedeclarableTemplateDecl : public TemplateDecl,
 ///
 /// The first value in the array is the number of specializations/partial
 /// specializations that follow.
-uint32_t *LazySpecializations = nullptr;
+LazySpecializationInfo *LazySpecializations = nullptr;
 
 /// The set of "injected" template arguments used within this
 /// template.
@@ -2268,7 +2296,7 @@ class ClassTemplateDecl : public RedeclarableTemplateDecl 
{
   friend class TemplateDeclInstantiator;
 
   /// Load any lazily-loaded specializations from the external sourc

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

@rjmccall @dwblaikie 

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

[clang] Issue #63106: [сlang] Representation of ellipsis in AST (PR #80976)

[Chuanqi Xu via cfe-commits]

@@ -41,7 +41,7 @@ void TestCatch2() {
   try {
   }
 // CHECK-NEXT:CXXCatchStmt
-// CHECK-NEXT:  NULL
+// CHECK-NEXT:  VarDecl {{.*}} ''

ChuanqiXu9 wrote:

Maybe we can improve to print this.

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

[clang] Issue #63106: [сlang] Representation of ellipsis in AST (PR #80976)

[Chuanqi Xu via cfe-commits]

@@ -1053,6 +1053,10 @@ class VarDecl : public DeclaratorDecl, public 
Redeclarable {
 LLVM_PREFERRED_TYPE(bool)
 unsigned ExceptionVar : 1;
 
+/// To Check the ellipsis
+LLVM_PREFERRED_TYPE(bool)
+unsigned EllipsisVar : 1;

ChuanqiXu9 wrote:

Should we move this to `ParmVarDeclBitfields`?

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

[clang] Issue #63106: [сlang] Representation of ellipsis in AST (PR #80976)

[Chuanqi Xu via cfe-commits]

@@ -16983,7 +16983,7 @@ VarDecl *Sema::BuildExceptionDeclaration(Scope *S,
 
 /// ActOnExceptionDeclarator - Parsed the exception-declarator in a C++ catch
 /// handler.
-Decl *Sema::ActOnExceptionDeclarator(Scope *S, Declarator &D) {
+Decl *Sema::ActOnExceptionDeclarator(Scope *S, Declarator &D, bool isCatchAll) 
{

ChuanqiXu9 wrote:

I am hesitate on this. IIUC, previously the `catch(...)` format is reprensented 
as the exception decl as nullptr? Then if yes, we should be able to reuse that.

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

[clang] Issue #63106: [сlang] Representation of ellipsis in AST (PR #80976)

[Chuanqi Xu via cfe-commits]

@@ -1053,6 +1053,10 @@ class VarDecl : public DeclaratorDecl, public 
Redeclarable {
 LLVM_PREFERRED_TYPE(bool)
 unsigned ExceptionVar : 1;
 
+/// To Check the ellipsis

ChuanqiXu9 wrote:

The comment is not clear

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

[clang] Issue #63106: [сlang] Representation of ellipsis in AST (PR #80976)

[Chuanqi Xu via cfe-commits]

@@ -2698,9 +2698,16 @@ StmtResult Parser::ParseCXXCatchBlock(bool FnCatch) {
 Declarator ExDecl(DS, Attributes, DeclaratorContext::CXXCatch);
 ParseDeclarator(ExDecl);
 ExceptionDecl = Actions.ActOnExceptionDeclarator(getCurScope(), ExDecl);
-  } else
-ConsumeToken();
-
+  }
+  else {
+  CatchLoc = ConsumeToken();
+  // explicitly creating a var of type no-type for '...' and marking it as 
catch_all
+  ParsedAttributes Attributes(AttrFactory);
+  DeclSpec DS(AttrFactory);
+  Declarator ExDecl(DS, Attributes, DeclaratorContext::BlockLiteral);
+  ParseDeclarator(ExDecl);
+  ExceptionDecl = Actions.ActOnExceptionDeclarator(getCurScope(), ExDecl, 
true);
+  }

ChuanqiXu9 wrote:

I am hesitate on this. IIUC, previously the catch(...) format is reprensented 
as the exception decl as nullptr? Then if yes, we should be able to reuse that.

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

[clang] Issue #63106: [сlang] Representation of ellipsis in AST (PR #80976)

[Chuanqi Xu via cfe-commits]

@@ -271,6 +271,9 @@ void TextNodeDumper::Visit(const Decl *D) {
   OS << " hidden";
   if (D->isImplicit())
 OS << " implicit";
+  if (const VarDecl *ND = dyn_cast(D))
+  if (ND->isEllipsisVariable())
+  OS << " catch_all";

ChuanqiXu9 wrote:

ditto

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

[clang] Issue #63106: [сlang] Representation of ellipsis in AST (PR #80976)

[Chuanqi Xu via cfe-commits]

@@ -115,6 +115,10 @@ void JSONNodeDumper::Visit(const Decl *D) {
   else if (D->isThisDeclarationReferenced())
 JOS.attribute("isReferenced", true);
 
+  if (const VarDecl *ND = dyn_cast(D))
+  if (ND->isEllipsisVariable())
+  JOS.attribute("catch_all", true);

ChuanqiXu9 wrote:

Format this.

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

[clang] Issue #63106: [сlang] Representation of ellipsis in AST (PR #80976)

[Chuanqi Xu via cfe-commits]

@@ -1474,6 +1478,16 @@ class VarDecl : public DeclaratorDecl, public 
Redeclarable {
 NonParmVarDeclBits.ExceptionVar = EV;
   }
 
+  /// Determine the Ellipsis (...) or not
+  bool isEllipsisVariable() const {
+return isa(this) ? false : NonParmVarDeclBits.EllipsisVar;
+  }
+  void setEllipsisVariable(bool EV) {
+assert(!isa(this));
+NonParmVarDeclBits.EllipsisVar = EV;
+  }

ChuanqiXu9 wrote:

Should we move this to ParmVarDecl?

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

[clang] Issue #63106: [сlang] Representation of ellipsis in AST (PR #80976)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

+1 that we should reduce the impact of the patch as much as possible. 

Also every time we change the data member of decls and stmts, we need to update 
the serialization part.

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

[clang] Issue #63106: [сlang] Representation of ellipsis in AST (PR #80976)

[Chuanqi Xu via cfe-commits]

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

[clang] clang serialization unittests: fix some leaks (PR #82773)

[Chuanqi Xu via cfe-commits]

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


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

[clang] b014944 - [NFC] [doc] Mentioning to include the guard headers from imported modules

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-02-23T16:54:13+08:00
New Revision: b014944e47ba6e2031e968268b15fba43a9e1dbf

URL: 
https://github.com/llvm/llvm-project/commit/b014944e47ba6e2031e968268b15fba43a9e1dbf
DIFF: 
https://github.com/llvm/llvm-project/commit/b014944e47ba6e2031e968268b15fba43a9e1dbf.diff

LOG: [NFC] [doc] Mentioning to include the guard headers from imported modules

Added: 


Modified: 
clang/docs/StandardCPlusPlusModules.rst

Removed: 




diff  --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index 0347ff077fdb87..c5478bba45f389 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -868,6 +868,9 @@ headers to:
   ...
   #endif
 
+If the modules imported by your library provides such headers too, remember to 
add them to
+your ``your_library_imported.h`` too.
+
 Importing modules
 ~
 



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 2e5af56 - [C++20] [Modules] Allow to compile a pcm with and without -fPIC

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-02-23T11:05:15+08:00
New Revision: 2e5af56b05c2d39ab2c829bf4c13190523b67ddd

URL: 
https://github.com/llvm/llvm-project/commit/2e5af56b05c2d39ab2c829bf4c13190523b67ddd
DIFF: 
https://github.com/llvm/llvm-project/commit/2e5af56b05c2d39ab2c829bf4c13190523b67ddd.diff

LOG: [C++20] [Modules] Allow to compile a pcm with and without -fPIC
seperately

We can compile a module unit in 2 phase compilaton:

```
clang++ -std=c++20 a.cppm --precompile -o a.pcm
clang++ -std=c++20 a.pcm -c -o a.o
```

And it is a general requirement that we need to compile a translation
unit with and without -fPIC for static and shared libraries.

But for C++20 modules with 2 phase compilation, it may be waste of time
to compile them 2 times completely. It may be fine to generate one BMI
and compile it with and without -fPIC seperately.

e.g.,

```
clang++ -std=c++20 a.cppm --precompile -o a.pcm
clang++ -std=c++20 a.pcm -c -o a.o
clang++ -std=c++20 a.pcm -c -fPIC -o a-PIC.o
```

Then we can save the time to parse a.cppm repeatedly.

Added: 
clang/test/Modules/compile-pcm-with-pic.cppm

Modified: 
clang/include/clang/Frontend/ASTUnit.h
clang/include/clang/Frontend/CompilerInstance.h
clang/include/clang/Frontend/CompilerInvocation.h
clang/lib/Frontend/ASTUnit.cpp
clang/lib/Frontend/FrontendAction.cpp
clang/tools/c-index-test/core_main.cpp
clang/tools/libclang/CIndex.cpp

Removed: 




diff  --git a/clang/include/clang/Frontend/ASTUnit.h 
b/clang/include/clang/Frontend/ASTUnit.h
index 6af712afdcb6d8..a2c1b25dd22476 100644
--- a/clang/include/clang/Frontend/ASTUnit.h
+++ b/clang/include/clang/Frontend/ASTUnit.h
@@ -691,16 +691,19 @@ class ASTUnit {
   /// lifetime is expected to extend past that of the returned ASTUnit.
   ///
   /// \returns - The initialized ASTUnit or null if the AST failed to load.
-  static std::unique_ptr LoadFromASTFile(
-  const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
-  WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
-  const FileSystemOptions &FileSystemOpts,
-  std::shared_ptr HSOpts, bool OnlyLocalDecls = false,
-  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
-  bool AllowASTWithCompilerErrors = false,
-  bool UserFilesAreVolatile = false,
-  IntrusiveRefCntPtr VFS =
-  llvm::vfs::getRealFileSystem());
+  static std::unique_ptr
+  LoadFromASTFile(const std::string &Filename,
+  const PCHContainerReader &PCHContainerRdr, WhatToLoad ToLoad,
+  IntrusiveRefCntPtr Diags,
+  const FileSystemOptions &FileSystemOpts,
+  std::shared_ptr HSOpts,
+  std::shared_ptr LangOpts = nullptr,
+  bool OnlyLocalDecls = false,
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
+  bool AllowASTWithCompilerErrors = false,
+  bool UserFilesAreVolatile = false,
+  IntrusiveRefCntPtr VFS =
+  llvm::vfs::getRealFileSystem());
 
 private:
   /// Helper function for \c LoadFromCompilerInvocation() and

diff  --git a/clang/include/clang/Frontend/CompilerInstance.h 
b/clang/include/clang/Frontend/CompilerInstance.h
index ac2f940769fbe9..b97d0c636806a9 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -311,6 +311,9 @@ class CompilerInstance : public ModuleLoader {
 
   LangOptions &getLangOpts() { return Invocation->getLangOpts(); }
   const LangOptions &getLangOpts() const { return Invocation->getLangOpts(); }
+  std::shared_ptr getLangOptsPtr() const {
+return Invocation->getLangOptsPtr();
+  }
 
   PreprocessorOptions &getPreprocessorOpts() {
 return Invocation->getPreprocessorOpts();

diff  --git a/clang/include/clang/Frontend/CompilerInvocation.h 
b/clang/include/clang/Frontend/CompilerInvocation.h
index c6528779bde7b2..8fc51e6ec03b64 100644
--- a/clang/include/clang/Frontend/CompilerInvocation.h
+++ b/clang/include/clang/Frontend/CompilerInvocation.h
@@ -271,6 +271,7 @@ class CompilerInvocation : public CompilerInvocationBase {
   std::shared_ptr getPreprocessorOptsPtr() {
 return PPOpts;
   }
+  std::shared_ptr getLangOptsPtr() { return LangOpts; }
   /// @}
 
   /// Create a compiler invocation from a list of input options.

diff  --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index f09a01b5dd4aff..3610a08831e79a 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -540,7 +540,17 @@ class ASTInfoCollector : public ASTReaderListener {
 if (InitializedLanguage)
   return false;
 
+// FIXME: We did similar things in ReadHeaderSearchOptions too. But such
+// style is not scaling. Probably we need to invite some mechanism to
+// handle such patterns generally.
+auto PICLe

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > > Let's zoom out a little. The approach in D41416 shows that it is 
> > > > feasible to store _a_ hash of the template arguments to delay eager 
> > > > deserializations. The ODR hash approach is a second order problem 
> > > > because we can swap it with something better once we need to. In order 
> > > > to make progress we have introduced 
> > > > [D153003](https://reviews.llvm.org/D153003) which allows our 
> > > > infrastructure to work. The way I see moving forward here is:
> > > > 
> > > > * Base this PR on D41416 in the approach how we model the lazy 
> > > > deserialization of templates. That'd mean that we "just" need to 
> > > > replace `LazySpecializationInfo *LazySpecializations = nullptr;` with 
> > > > the on-disk hash table approach. That would probably require 
> > > > centralizing that logic somewhere in the ASTReader (the way this PR 
> > > > does) but with minimal changes wrt D41416.
> > > > * Test the implementation on our infrastructure for correctness
> > > > * Test the implementation on the Google infrastructure for scalability
> > > > * Think on a better approach to replace odr hashing if we see more 
> > > > pathological problems.
> > > 
> > > 
> > > Yeah, no problem at all. This is what I want in the higher level too. 
> > > What I am confused is about the status of 
> > > [D153003](https://reviews.llvm.org/D153003). If it is true that we've 
> > > describe the problem completely in the review page, then 
> > > [c31d6b4](https://github.com/llvm/llvm-project/commit/c31d6b4ef135098280b0ebb93e95b258a0d372ca)
> > >  should be a proper fix for that.
> > 
> > 
> > I can try it on our infrastructure and if it works I will remove D153003.
> 
> @ChuanqiXu9, you were right. We seem to not need D153003 and I have removed 
> it from the branch.

Yeah, then let's create a new branch (the existing 
`[D41416_D153003](https://github.com/llvm/llvm-project/tree/users/vgvassilev/D41416_D153003)`
 sounds not like a good name) and a PR for that. Then I can start a stacked PR 
on that.

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

[clang] 96e5657 - [NFC] [Modules] Add a test for issue 81745

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-02-20T14:33:48+08:00
New Revision: 96e56573089b2a211c71660b0ffc7deb21049bdd

URL: 
https://github.com/llvm/llvm-project/commit/96e56573089b2a211c71660b0ffc7deb21049bdd
DIFF: 
https://github.com/llvm/llvm-project/commit/96e56573089b2a211c71660b0ffc7deb21049bdd.diff

LOG: [NFC] [Modules] Add a test for issue 81745

Although the root cause of
https://github.com/llvm/llvm-project/issues/81745 shows not related to
modules, it should be good to add a regression test for that.

Added: 
clang/test/Modules/pr81745.cppm

Modified: 


Removed: 




diff  --git a/clang/test/Modules/pr81745.cppm b/clang/test/Modules/pr81745.cppm
new file mode 100644
index 00..4246d860c8e0d2
--- /dev/null
+++ b/clang/test/Modules/pr81745.cppm
@@ -0,0 +1,23 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/M.cppm  -triple=x86_64-linux-gnu \
+// RUN: -emit-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/foo.cpp -fprebuilt-module-path=%t \
+// RUN:  -triple=x86_64-linux-gnu  -emit-llvm -o - | FileCheck %t/foo.cpp
+
+//--- M.cppm
+export module M;
+export struct S1 {
+consteval S1(int) {}
+};
+
+//--- foo.cpp
+import M;
+void foo() {
+struct S2 { S1 s = 0; };
+S2 s;
+}
+
+// CHECK-NOT: _ZNW1M2S1C1Ei



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clangd] Fix C++20 modules crash (PR #81919)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

I'd like to close this since the issue got closed.

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

[clang] [clangd] Fix C++20 modules crash (PR #81919)

[Chuanqi Xu via cfe-commits]

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

[clang] [Serialization] Record whether the ODR is skipped (PR #82302)

[Chuanqi Xu via cfe-commits]

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

[clang] [Serialization] Record whether the ODR is skipped (PR #82302)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

The CI failure looks unrelated to this:

```
CMake Error at 
C:/BuildTools/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/CMakeDetermineCompilerId.cmake:777
 (file):
--
  | file STRINGS file
  | 
"C:/ws/src/build/build-clang-windows/CMakeFiles/3.20.21032501-MSVC_2/CompilerIdC/CMakeCCompilerId.exe"
  | cannot be read.
  | Call Stack (most recent call first):
  | 
C:/BuildTools/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/CMakeDetermineCompilerId.cmake:11
 (CMAKE_DETERMINE_COMPILER_ID_CHECK)
  | 
C:/BuildTools/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/CMakeDetermineCompilerId.cmake:58
 (__determine_compiler_id_test)
  | 
C:/BuildTools/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/CMakeDetermineCCompiler.cmake:118
 (CMAKE_DETERMINE_COMPILER_ID)
  | CMakeLists.txt:54 (project)
```

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

[clang] [docs] [C++20] [Modules] Ideas for transitioning to modules (PR #80687)

[Chuanqi Xu via cfe-commits]

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

[clang] [docs] [C++20] [Modules] Ideas for transitioning to modules (PR #80687)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/80687

>From 19065e2e25e8bbd735b157239702e4394659bbc7 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Mon, 5 Feb 2024 22:31:19 +0800
Subject: [PATCH 1/2] [docs] [C++20] [Modules] Ideas for transiting to modules

---
 clang/docs/StandardCPlusPlusModules.rst | 339 
 1 file changed, 339 insertions(+)

diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index c322805d8db5bc..f4c3c4f0d8813c 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -610,6 +610,345 @@ the following style significantly:
 
 The key part of the tip is to reduce the duplications from the text includes.
 
+Ideas for converting to modules
+---
+
+For new libraries, we encourage them to use modules completely from day one if 
possible.
+This will be pretty helpful to make the whole ecosystems to get ready.
+
+For many existing libraries, it may be a breaking change to refactor themselves
+into modules completely. So that many existing libraries need to provide 
headers and module
+interfaces for a while to not break existing users.
+Here we provide some ideas to ease the transition process for existing 
libraries.
+**Note that the this section is only about helping ideas instead of 
requirement from clang**.
+
+Let's start with the case that there is no dependency or no dependent 
libraries providing
+modules for your library.
+
+ABI non-breaking styles
+~~~
+
+export-using style
+^^
+
+.. code-block:: c++
+
+  module;
+  #include "header_1.h"
+  #include "header_2.h"
+  ...
+  #include "header_n.h"
+  export module your_library;
+  export namespace your_namespace {
+using decl_1;
+using decl_2;
+...
+using decl_n;
+  }
+
+As the example shows, you need to include all the headers containing 
declarations needs
+to be exported and `using` such declarations in an `export` block. Then, 
basically,
+we're done.
+
+export extern-C++ style
+^^^
+
+.. code-block:: c++
+
+  module;
+  #include "third_party/A/headers.h"
+  #include "third_party/B/headers.h"
+  ...
+  #include "third_party/Z/headers.h"
+  export module your_library;
+  #define IN_MODULE_INTERFACE
+  extern "C++" {
+#include "header_1.h"
+#include "header_2.h"
+...
+#include "header_n.h"
+  }
+
+Then in your headers (from ``header_1.h`` to ``header_n.h``), you need to 
define the macro:
+
+.. code-block:: c++
+
+  #ifdef IN_MODULE_INTERFACE
+  #define EXPORT export
+  #else
+  #define EXPORT
+  #endif
+
+And you should put ``EXPORT`` to the beginning of the declarations you want to 
export.
+
+Also it is suggested to refactor your headers to include thirdparty headers 
conditionally:
+
+.. code-block:: c++
+
+  + #ifndef IN_MODULE_INTERFACE
+  #include "third_party/A/headers.h"
+  + #endif
+
+  #include "header_x.h"
+
+  ...
+
+This may be helpful to get better diagnostic messages if you forgot to update 
your module 
+interface unit file during maintaining.
+
+The reasoning for the practice is that the declarations in the language 
linkage are considered
+to be attached to the global module. So the ABI of your library in the modular 
version
+wouldn't change.
+
+While this style looks not as convenient as the export-using style, it is 
easier to convert 
+to other styles.
+
+ABI breaking style
+~~
+
+The term ``ABI breaking`` sounds terrifying generally. But you may want it 
here if you want
+to force your users to introduce your library in a consistent way. E.g., they 
either include
+your headers all the way or import your modules all the way.
+The style prevents the users to include your headers and import your modules 
at the same time
+in the same repo.
+
+The pattern for ABI breaking style is similar with export extern-C++ style.
+
+.. code-block:: c++
+
+  module;
+  #include "third_party/A/headers.h"
+  #include "third_party/B/headers.h"
+  ...
+  #include "third_party/Z/headers.h"
+  export module your_library;
+  #define IN_MODULE_INTERFACE
+  #include "header_1.h"
+  #include "header_2.h"
+  ...
+  #include "header_n.h"
+
+  #if the number of .cpp files in your project are small
+  module :private;
+  #include "source_1.cpp"
+  #include "source_2.cpp"
+  ...
+  #include "source_n.cpp"
+  #else // the number of .cpp files in your project are a lot
+  // Using all the declarations from thirdparty libraries which are
+  // used in the .cpp files.
+  namespace third_party_namespace {
+using third_party_decl_used_in_cpp_1;
+using third_party_decl_used_in_cpp_2;
+...
+using third_party_decl_used_in_cpp_n;
+  }
+  #endif
+
+(And add `EXPORT` and conditional include to the headers as suggested in the 
export
+extern-C++ style section)
+
+Remember that the ABI get changed and we need to compile our source files into 
the
+new ABI format. Thi

[clang] [Serialization] Record whether the ODR is skipped (PR #82302)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 created 
https://github.com/llvm/llvm-project/pull/82302

Close https://github.com/llvm/llvm-project/issues/80570.

In
https://github.com/llvm/llvm-project/commit/a0b6747804e46665ecfd00295b60432bfe1775b6,
 we skipped ODR checks for decls in GMF. Then it should be natural to skip 
storing the ODR values in BMI.

Generally it should be fine as long as the writer and the reader keep 
consistent.

However, the use of preamble in clangd shows the tricky part.

For,

```
// test.cpp
module;

// any one off these is enough to crash clangd
// #include 
// #include 
// #include 
// #include 
// #include 
// #include 
// probably many more

// only ok with libc++, not the system provided libstdc++ 13.2.1

// these are ok

export module test;
```

clangd will store the headers as preamble to speedup the parsing and the 
preamble reuses the serialization techniques. (Generally we'd call the preamble 
as PCH. However it is not true strictly. I've tested the PCH wouldn't be 
problematic.) However, the tricky part is that the preamble is not modules. It 
literally serialiaze and deserialize things. So before clangd parsing the above 
test module, clangd will serialize the headers into the preamble. Note that 
there is no concept like GMF now. So the ODR bits are stored. However, when 
clangd parse the file actually, the decls from preamble are thought as in GMF 
literally, then hte ODR bits are skipped. Then mismatch happens.

To solve the problem, this patch adds another bit for decls to record whether 
or not the ODR bits are skipped.

>From 9841b09e27f32c26729247b2dc3160cf145fd066 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Tue, 20 Feb 2024 10:48:55 +0800
Subject: [PATCH] [Serialization] Record whether the ODR is skipped

Close https://github.com/llvm/llvm-project/issues/80570.

In
https://github.com/llvm/llvm-project/commit/a0b6747804e46665ecfd00295b60432bfe1775b6,
we skipped ODR checks for decls in GMF. Then it should be natural to
skip storing the ODR values in BMI.

Generally it should be fine as long as the writer and the reader keep
consistent.

However, the use of preamble in clangd shows the tricky part.

For,

```
// test.cpp
module;

// any one off these is enough to crash clangd
// #include 
// #include 
// #include 
// #include 
// #include 
// #include 
// probably many more

// only ok with libc++, not the system provided libstdc++ 13.2.1

// these are ok

export module test;
```

clangd will store the headers as preamble to speedup the parsing and the
preamble reuses the serialization techniques. (Generally we'd call the
preamble as PCH. However it is not true strictly. I've tested the PCH
wouldn't be problematic.) However, the tricky part is that the preamble
is not modules. It literally serialiaze and deserialize things. So
before clangd parsing the above test module, clangd will serialize the
headers into the preamble. Note that there is no concept like GMF now.
So the ODR bits are stored. However, when clangd parse the file
actually, the decls from preamble are thought as in GMF literally, then
hte ODR bits are skipped. Then mismatch happens.

To solve the problem, this patch adds another bit for decls to record whether or
not the ODR bits are skipped.
---
 clang/lib/Serialization/ASTReaderDecl.cpp |  10 +-
 clang/lib/Serialization/ASTWriter.cpp |   6 +-
 clang/lib/Serialization/ASTWriterDecl.cpp |  15 +-
 clang/unittests/Serialization/CMakeLists.txt  |   1 +
 .../PreambleInNamedModulesTest.cpp| 132 ++
 5 files changed, 154 insertions(+), 10 deletions(-)
 create mode 100644 clang/unittests/Serialization/PreambleInNamedModulesTest.cpp

diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp 
b/clang/lib/Serialization/ASTReaderDecl.cpp
index ffba04f28782ea..d5309e3fc31f70 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -800,11 +800,12 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
   BitsUnpacker EnumDeclBits(Record.readInt());
   ED->setNumPositiveBits(EnumDeclBits.getNextBits(/*Width=*/8));
   ED->setNumNegativeBits(EnumDeclBits.getNextBits(/*Width=*/8));
+  bool ShouldSkipCheckingODR = EnumDeclBits.getNextBit();
   ED->setScoped(EnumDeclBits.getNextBit());
   ED->setScopedUsingClassTag(EnumDeclBits.getNextBit());
   ED->setFixed(EnumDeclBits.getNextBit());
 
-  if (!shouldSkipCheckingODR(ED)) {
+  if (!ShouldSkipCheckingODR) {
 ED->setHasODRHash(true);
 ED->ODRHash = Record.readInt();
   }
@@ -1073,6 +1074,7 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
 
   FD->setCachedLinkage((Linkage)FunctionDeclBits.getNextBits(/*Width=*/3));
   FD->setStorageClass((StorageClass)FunctionDeclBits.getNextBits(/*Width=*/3));
+  bool ShouldSkipCheckingODR = FunctionDeclBits.getNextBit();
   FD->setInlineSpecified(FunctionDeclBits.getNextBit());
   FD->setImplicitlyInline(FunctionDeclBits.getNextBit());
   FD->setHasSkippedBody(FunctionDeclBits.getNext

[clang] [Clang] CXXConstructExpr may be immediate calls. (PR #82179)

[Chuanqi Xu via cfe-commits]

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

LGTM.

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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> Let's zoom out a little. The approach in D41416 shows that it is feasible to 
> store _a_ hash of the template arguments to delay eager deserializations. The 
> ODR hash approach is a second order problem because we can swap it with 
> something better once we need to. In order to make progress we have 
> introduced [D153003](https://reviews.llvm.org/D153003) which allows our 
> infrastructure to work. The way I see moving forward here is:
> 
> * Base this PR on D41416 in the approach how we model the lazy 
> deserialization of templates. That'd mean that we "just" need to replace 
> `LazySpecializationInfo *LazySpecializations = nullptr;` with the on-disk 
> hash table approach. That would probably require centralizing that logic 
> somewhere in the ASTReader (the way this PR does) but with minimal changes 
> wrt D41416.
> * Test the implementation on our infrastructure for correctness
> * Test the implementation on the Google infrastructure for scalability
> * Think on a better approach to replace odr hashing if we see more 
> pathological problems.

Yeah, no problem at all. This is what I want in the higher level too. What I am 
confused is about the status of [D153003](https://reviews.llvm.org/D153003). If 
it is true that we've describe the problem completely in the review page, then 
https://github.com/llvm/llvm-project/commit/c31d6b4ef135098280b0ebb93e95b258a0d372ca
 should be a proper fix for that.

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

[clang] Reland "[clang][modules] Print library module manifest path." (PR #82160)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,38 @@
+// Test that -print-library-module-manifest-path finds the correct file.
+
+// REQUIRES: x86-registered-target

ChuanqiXu9 wrote:

```suggestion
// FIXME: 
// REQUIRES: x86-registered-target
```

I feel better with a FIXME to remind us to remove this later.

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

[clang] Reland "[clang][modules] Print library module manifest path." (PR #82160)

[Chuanqi Xu via cfe-commits]

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

LGTM.

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

[clang] Reland "[clang][modules] Print library module manifest path." (PR #82160)

[Chuanqi Xu via cfe-commits]

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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > But my point is that we can't land that if we don't understand what's going 
> > wrong without that patch.
> 
> We understand that very well and it's described in 
> https://reviews.llvm.org/D153003 as well as the surrounding discussions: 
> because of the way that `ODRHash` works, template template arguments `A` and 
> `B` will hash to different values, even if `using A = B`. 

Yeah, so I tried to fix that in the following patches. And if that works, I 
expect that can fix internal errors in your workloads.

> However, for template specializations, we require them to hash to the same 
> value (with some form of normalization) or we won't find nor load the right 
> specializations. That's why I said that IMHO `ODRHash` is not the right tool 
> for the job here, which follows directly from an old comment of yours: 
> https://reviews.llvm.org/D153003#4427412
> 
> > An important node here is that ODRHash is used to check the AST Nodes are 
> > keeping the same across compilations. There is gap to use ODRHash to check 
> > the semantical equality.
> 
> (and IIRC that's the same direction that Richard was going)



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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > > > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy 
> > > > > > > > > > template specialization deserialization mechanism 
> > > > > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > From 
> > > > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495),
> > > > > > > > >  I see there is new reply saying the testing is actually 
> > > > > > > > > fine. Do you think we still need to split the patch?
> > > > > > > > 
> > > > > > > > 
> > > > > > > > That comment was concerning the version of the patch that had 
> > > > > > > > the lazy template deserialization turned off by default. Yes, I 
> > > > > > > > still think that this patch should implement tha on-disk hash 
> > > > > > > > table on top of D41416
> > > > > > > 
> > > > > > > 
> > > > > > > OK. And would you like to send a PR for D41416? I've already 
> > > > > > > fixed the issue mentioned in the review page. Then I'd like to 
> > > > > > > send small and incremental patches on that.
> > > > > > 
> > > > > > 
> > > > > > Do you mean that I should open a PR for D41416 and you will apply 
> > > > > > your patch there? I have no problem if we do everything here as 
> > > > > > part of this PR. This way we will have the full history of how this 
> > > > > > was born in one place ;)
> > > > > 
> > > > > 
> > > > > Yeah, and please create a branch under llvm/llvm-project directly. 
> > > > > Then I can perform stacked PR on that.
> > > > 
> > > > 
> > > > There it is: 
> > > > https://github.com/llvm/llvm-project/tree/users/vgvassilev/D41416_D153003
> > > 
> > > 
> > > If I drop it then our tests will break. IIUC that's somewhere deep in the 
> > > hasher and should be not impact this PR. Does this make the work on the 
> > > on-disk hashtable more complicated in some way?
> > 
> > 
> > No, it won't block the work for on-disk hashtable. But if we want to land 
> > that, we must understand what happened actually...
> 
> We can’t land that without attaching your on-disk hashtable implementation 
> part of this PR because of what’s mentioned here [#76774 
> (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1893222165)

I know that. But we're not talking about the same thing. This is one of the 
reason that we can't land that. But my point is that we can't land that if we 
don't understand what's going wrong without that patch.

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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template 
> > > > > > > > specialization deserialization mechanism 
> > > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > > > 
> > > > > > > 
> > > > > > > From 
> > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495),
> > > > > > >  I see there is new reply saying the testing is actually fine. Do 
> > > > > > > you think we still need to split the patch?
> > > > > > 
> > > > > > 
> > > > > > That comment was concerning the version of the patch that had the 
> > > > > > lazy template deserialization turned off by default. Yes, I still 
> > > > > > think that this patch should implement tha on-disk hash table on 
> > > > > > top of D41416
> > > > > 
> > > > > 
> > > > > OK. And would you like to send a PR for D41416? I've already fixed 
> > > > > the issue mentioned in the review page. Then I'd like to send small 
> > > > > and incremental patches on that.
> > > > 
> > > > 
> > > > Do you mean that I should open a PR for D41416 and you will apply your 
> > > > patch there? I have no problem if we do everything here as part of this 
> > > > PR. This way we will have the full history of how this was born in one 
> > > > place ;)
> > > 
> > > 
> > > Yeah, and please create a branch under llvm/llvm-project directly. Then I 
> > > can perform stacked PR on that.
> > 
> > 
> > There it is: 
> > https://github.com/llvm/llvm-project/tree/users/vgvassilev/D41416_D153003
> 
> If I drop it then our tests will break. IIUC that's somewhere deep in the 
> hasher and should be not impact this PR. Does this make the work on the 
> on-disk hashtable more complicated in some way?

No, it won't block the work for on-disk hashtable. But if we want to land that, 
we must understand what happened actually...

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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template 
> > > > > > > > > specialization deserialization mechanism 
> > > > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > > > > 
> > > > > > > > 
> > > > > > > > From 
> > > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495),
> > > > > > > >  I see there is new reply saying the testing is actually fine. 
> > > > > > > > Do you think we still need to split the patch?
> > > > > > > 
> > > > > > > 
> > > > > > > That comment was concerning the version of the patch that had the 
> > > > > > > lazy template deserialization turned off by default. Yes, I still 
> > > > > > > think that this patch should implement tha on-disk hash table on 
> > > > > > > top of D41416
> > > > > > 
> > > > > > 
> > > > > > OK. And would you like to send a PR for D41416? I've already fixed 
> > > > > > the issue mentioned in the review page. Then I'd like to send small 
> > > > > > and incremental patches on that.
> > > > > 
> > > > > 
> > > > > Do you mean that I should open a PR for D41416 and you will apply 
> > > > > your patch there? I have no problem if we do everything here as part 
> > > > > of this PR. This way we will have the full history of how this was 
> > > > > born in one place ;)
> > > > 
> > > > 
> > > > Yeah, and please create a branch under llvm/llvm-project directly. Then 
> > > > I can perform stacked PR on that.
> > > 
> > > 
> > > There it is: 
> > > https://github.com/llvm/llvm-project/tree/users/vgvassilev/D41416_D153003
> > 
> > 
> > Is there a PR? So that we can comment on that. For example, is D153003 
> > necessary? I remeber in the review process, we think it may not be wanted.
> 
> I can create a PR. My understanding is that D153003 because of the 
> intricacies of the ODR hashing approach. If we create an alternative we can 
> take it out. For now, we can keep it and later we can drop it.

I feel better to drop D153003 if it is not a blocking issue. I feel it make 
things more complicated...

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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template 
> > > > > > > specialization deserialization mechanism 
> > > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > > 
> > > > > > 
> > > > > > From 
> > > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495),
> > > > > >  I see there is new reply saying the testing is actually fine. Do 
> > > > > > you think we still need to split the patch?
> > > > > 
> > > > > 
> > > > > That comment was concerning the version of the patch that had the 
> > > > > lazy template deserialization turned off by default. Yes, I still 
> > > > > think that this patch should implement tha on-disk hash table on top 
> > > > > of D41416
> > > > 
> > > > 
> > > > OK. And would you like to send a PR for D41416? I've already fixed the 
> > > > issue mentioned in the review page. Then I'd like to send small and 
> > > > incremental patches on that.
> > > 
> > > 
> > > Do you mean that I should open a PR for D41416 and you will apply your 
> > > patch there? I have no problem if we do everything here as part of this 
> > > PR. This way we will have the full history of how this was born in one 
> > > place ;)
> > 
> > 
> > Yeah, and please create a branch under llvm/llvm-project directly. Then I 
> > can perform stacked PR on that.
> 
> There it is: 
> https://github.com/llvm/llvm-project/tree/users/vgvassilev/D41416_D153003

Is there a PR? So that we can comment on that. For example, is D153003 
necessary? I remeber in the review process, we think it may not be wanted.

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

[clang] [Clang] Unify interface for accessing template arguments as written for class/variable template specializations (PR #81642)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

Sounds good. My  instinct reaction to the title is that you're going to unify 
the annoying duplicated interfaces for the 5 specialization classes (function 
specialization, class/var (partial) specializations). But it is still good to 
merge these things.

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

[clang] [Clang] Unify interface for accessing template arguments as written for class/variable template specializations (PR #81642)

[Chuanqi Xu via cfe-commits]

@@ -1792,23 +1807,11 @@ class ClassTemplateSpecializationDecl
   llvm::PointerUnion
 SpecializedTemplate;
 
-  /// Further info for explicit template specialization/instantiation.
-  struct ExplicitSpecializationInfo {
-/// The type-as-written.
-TypeSourceInfo *TypeAsWritten = nullptr;
-
-/// The location of the extern keyword.
-SourceLocation ExternLoc;
-
-/// The location of the template keyword.
-SourceLocation TemplateKeywordLoc;
-
-ExplicitSpecializationInfo() = default;
-  };
-
   /// Further info for explicit template specialization/instantiation.
   /// Does not apply to implicit specializations.
-  ExplicitSpecializationInfo *ExplicitInfo = nullptr;
+  llvm::PointerUnion
+  ExplicitInfo = nullptr;

ChuanqiXu9 wrote:

I spent some time to understand `const ASTTemplateArgumentListInfo *` is only 
meaningful for partial specializations. Can we reuse 
ExplicitInstantiationInfo::TemplateArgsAsWrittenTemplateArgsAsWritten? And make 
`ExplicitInstantiationInfo` to `InstantiationInfo` so that we can use it 
generally?

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

[clang] [Clang] Unify interface for accessing template arguments as written for class/variable template specializations (PR #81642)

[Chuanqi Xu via cfe-commits]

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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template 
> > > > > specialization deserialization mechanism 
> > > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > 
> > > > 
> > > > From 
> > > > [root-project/root#14495](https://github.com/root-project/root/pull/14495),
> > > >  I see there is new reply saying the testing is actually fine. Do you 
> > > > think we still need to split the patch?
> > > 
> > > 
> > > That comment was concerning the version of the patch that had the lazy 
> > > template deserialization turned off by default. Yes, I still think that 
> > > this patch should implement tha on-disk hash table on top of D41416
> > 
> > 
> > OK. And would you like to send a PR for D41416? I've already fixed the 
> > issue mentioned in the review page. Then I'd like to send small and 
> > incremental patches on that.
> 
> Do you mean that I should open a PR for D41416 and you will apply your patch 
> there? I have no problem if we do everything here as part of this PR. This 
> way we will have the full history of how this was born in one place ;)

Yeah, and please create a branch under llvm/llvm-project directly. Then I can 
perform stacked PR on that.

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

[clang] 1ecbab5 - [C++20] [Modules] Don't import non-inline function bodies even if it is marked as always_inline

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2024-02-18T15:15:28+08:00
New Revision: 1ecbab56dcbb78268c8d19af34a50591f90b12a0

URL: 
https://github.com/llvm/llvm-project/commit/1ecbab56dcbb78268c8d19af34a50591f90b12a0
DIFF: 
https://github.com/llvm/llvm-project/commit/1ecbab56dcbb78268c8d19af34a50591f90b12a0.diff

LOG: [C++20] [Modules] Don't import non-inline function bodies even if it is 
marked as always_inline

Close https://github.com/llvm/llvm-project/issues/80949

Previously, I thought the always-inline function can be an exception to
enable optimizations as much as possible. However, it looks like it
breaks the ABI requirement we discussed later. So it looks better to not
import non-inline function bodies at all even if the function bodies are
marked as always_inline.

It doesn't produce regressions in some degree since the always_inline
still works in the same TU.

Added: 


Modified: 
clang/lib/CodeGen/CodeGenModule.cpp
clang/test/CodeGenCXX/module-funcs-from-imports.cppm

Removed: 




diff  --git a/clang/lib/CodeGen/CodeGenModule.cpp 
b/clang/lib/CodeGen/CodeGenModule.cpp
index c984260b082cd1..836cd34a16c0a1 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -3985,8 +3985,7 @@ bool CodeGenModule::shouldEmitFunction(GlobalDecl GD) {
   // behavior may break ABI compatibility of the current unit.
   if (const Module *M = F->getOwningModule();
   M && M->getTopLevelModule()->isNamedModule() &&
-  getContext().getCurrentNamedModule() != M->getTopLevelModule() &&
-  !F->hasAttr())
+  getContext().getCurrentNamedModule() != M->getTopLevelModule())
 return false;
 
   if (F->hasAttr())

diff  --git a/clang/test/CodeGenCXX/module-funcs-from-imports.cppm 
b/clang/test/CodeGenCXX/module-funcs-from-imports.cppm
index 33cdf437110a9e..8d04328eaf3fea 100644
--- a/clang/test/CodeGenCXX/module-funcs-from-imports.cppm
+++ b/clang/test/CodeGenCXX/module-funcs-from-imports.cppm
@@ -53,11 +53,11 @@ int use() {
 return exported_func() + always_inline_func();
 }
 
-// Checks that none of the function (except the always_inline_func) in the 
importees
+// Checks that none of the function in the importees
 // are generated in the importer's code.
 // CHECK-O0: define{{.*}}_Z3usev(
 // CHECK-O0: declare{{.*}}_ZW1M13exported_funcv(
-// CHECK-O0: define{{.*}}available_externally{{.*}}_ZW1M18always_inline_funcv(
+// CHECK-O0: declare{{.*}}_ZW1M18always_inline_funcv(
 // CHECK-O0-NOT: func_in_gmf
 // CHECK-O0-NOT: func_in_gmf_not_called
 // CHECK-O0-NOT: non_exported_func
@@ -68,7 +68,7 @@ int use() {
 // O0 to keep consistent ABI.
 // CHECK-O1: define{{.*}}_Z3usev(
 // CHECK-O1: declare{{.*}}_ZW1M13exported_funcv(
-// CHECK-O1: define{{.*}}available_externally{{.*}}_ZW1M18always_inline_funcv(
+// CHECK-O1: declare{{.*}}_ZW1M18always_inline_funcv(
 // CHECK-O1-NOT: func_in_gmf
 // CHECK-O1-NOT: func_in_gmf_not_called
 // CHECK-O1-NOT: non_exported_func



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> > > [do not merge] [runtime-cxxmodules] Rework our lazy template 
> > > specialization deserialization mechanism 
> > > [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > 
> > 
> > From 
> > [root-project/root#14495](https://github.com/root-project/root/pull/14495), 
> > I see there is new reply saying the testing is actually fine. Do you think 
> > we still need to split the patch?
> 
> That comment was concerning the version of the patch that had the lazy 
> template deserialization turned off by default. Yes, I still think that this 
> patch should implement tha on-disk hash table on top of D41416

OK. And would you like to send a PR for D41416? I've already fixed the issue 
mentioned in the review page. Then I'd like to send small and incremental 
patches on that.

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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> [do not merge] [runtime-cxxmodules] Rework our lazy template specialization 
> deserialization mechanism root-project/root#14495

>From https://github.com/root-project/root/pull/14495, I see there is new reply 
>saying the testing is actually fine. Do you think we still need to split the 
>patch?

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

[clang] [C++20] [Modules] Introduce reduced BMI (PR #75894)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

@iains @mizvekov ping~

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

[clang] [clang][modules] Print library module manifest path. (PR #76451)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

(This is another example that the github review can't work well with the 
reverted patches...)

@mordante I think you can add `// REQUIRES: x86-registered-target` to the test 
if @kaz7 can't respond quickly. It will skip the test on targets other than 
x86. And it should keep the CI green and enable us to backport this clang18.

We can still improve this later.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -5097,6 +5099,22 @@ class CoroutineSuspendExpr : public Expr {
 return static_cast(SubExprs[SubExpr::Operand]);
   }
 
+  SuspendReturnType getSuspendReturnType() const {
+auto *SuspendExpr = getSuspendExpr();
+assert(SuspendExpr);
+
+auto SuspendType = SuspendExpr->getType();
+
+if (SuspendType->isVoidType())
+  return SuspendReturnType::SuspendVoid;
+if (SuspendType->isBooleanType())
+  return SuspendReturnType::SuspendBool;
+if (SuspendType->isVoidPointerType())
+  return SuspendReturnType::SuspendHandle;
+
+llvm_unreachable("Unexpected await_suspend expression return type");

ChuanqiXu9 wrote:

```suggestion
return SuspendReturnType::SuspendHandle;
```

the `void pointer type` part looks odd.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -232,16 +250,74 @@ static LValueOrRValue 
emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
   auto *NullPtr = llvm::ConstantPointerNull::get(CGF.CGM.Int8PtrTy);
   auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr});
 
+  const auto AwaitSuspendCanThrow =
+  AwaitSuspendStmtCanThrow(S.getSuspendExpr());
+
+  auto SuspendWrapper = CodeGenFunction(CGF.CGM).generateAwaitSuspendWrapper(
+  CGF.CurFn->getName(), Prefix, S);
+
+  llvm::CallBase *SuspendRet = nullptr;

ChuanqiXu9 wrote:

Let's sink the definitions to the uses.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -1396,9 +1478,18 @@ static bool simplifySuspendPoint(CoroSuspendInst 
*Suspend,
   if (!SubFn)
 return false;
 
-  // Does not refer to the current coroutine, we cannot do anything with it.
-  if (SubFn->getFrame() != CoroBegin)
-return false;
+  auto Frame = SubFn->getFrame();
+
+  // Check that frame directly always refers to the current coroutine,
+  // either directly or via wrapper
+  if (Frame != CoroBegin) {
+auto *AWS = dyn_cast(Frame);
+if (!AWS)
+  return false;

ChuanqiXu9 wrote:

Since we've lowered all `CoroAwaitSuspendInst` before invoking 
`splitCoroutine`. We shouldn't see `CoroAwaitSuspendInst` here, right?

Then we can remove  isSimpleWrapper too.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -5038,6 +5038,8 @@ class CoroutineSuspendExpr : public Expr {
   OpaqueValueExpr *OpaqueValue = nullptr;
 
 public:
+  enum SuspendReturnType { SuspendVoid, SuspendBool, SuspendHandle };

ChuanqiXu9 wrote:

nit: Add a comment to explain that the return type of coroutines can only be 
one of them. Also I prefer `enum class` style.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

Thanks. This looks much better now.

Given the CoroAwaitSuspendInst is lowered before splitting coroutines, I think 
we don't need to handle it specially in `CoroSplit` any more.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -2013,6 +2104,10 @@ splitCoroutine(Function &F, SmallVectorImpl 
&Clones,
   buildCoroutineFrame(F, Shape, MaterializableCallback);
   replaceFrameSizeAndAlignment(Shape);
 
+  IRBuilder<> Builder(M.getContext());

ChuanqiXu9 wrote:

```suggestion
  IRBuilder<> Builder(F.getContext());
```

then we can get rid of `Module`.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -380,66 +380,7 @@ static Expr *maybeTailCall(Sema &S, QualType RetType, Expr 
*E,
   // __builtin_coro_resume so that the cleanup code are not inserted in-between
   // the resume call and return instruction, which would interfere with the
   // musttail call contract.
-  JustAddress = S.MaybeCreateExprWithCleanups(JustAddress);
-  return S.BuildBuiltinCallExpr(Loc, Builtin::BI__builtin_coro_resume,
-JustAddress);
-}
-
-/// The await_suspend call performed by co_await is essentially asynchronous
-/// to the execution of the coroutine. Inlining it normally into an unsplit
-/// coroutine can cause miscompilation because the coroutine CFG misrepresents
-/// the true control flow of the program: things that happen in the
-/// await_suspend are not guaranteed to happen prior to the resumption of the
-/// coroutine, and things that happen after the resumption of the coroutine
-/// (including its exit and the potential deallocation of the coroutine frame)
-/// are not guaranteed to happen only after the end of await_suspend.
-///
-/// See https://github.com/llvm/llvm-project/issues/56301 and
-/// https://reviews.llvm.org/D157070 for the example and the full discussion.
-///
-/// The short-term solution to this problem is to mark the call as uninlinable.
-/// But we don't want to do this if the call is known to be trivial, which is
-/// very common.
-///
-/// The long-term solution may introduce patterns like:
-///
-///  call @llvm.coro.await_suspend(ptr %awaiter, ptr %handle,
-///ptr @awaitSuspendFn)
-///
-/// Then it is much easier to perform the safety analysis in the middle end.
-/// If it is safe to inline the call to awaitSuspend, we can replace it in the
-/// CoroEarly pass. Otherwise we could replace it in the CoroSplit pass.
-static void tryMarkAwaitSuspendNoInline(Sema &S, OpaqueValueExpr *Awaiter,
-CallExpr *AwaitSuspend) {
-  // The method here to extract the awaiter decl is not precise.
-  // This is intentional. Since it is hard to perform the analysis in the
-  // frontend due to the complexity of C++'s type systems.
-  // And we prefer to perform such analysis in the middle end since it is
-  // easier to implement and more powerful.
-  CXXRecordDecl *AwaiterDecl =
-  Awaiter->getType().getNonReferenceType()->getAsCXXRecordDecl();
-
-  if (AwaiterDecl && AwaiterDecl->field_empty())
-return;
-
-  FunctionDecl *FD = AwaitSuspend->getDirectCallee();
-
-  assert(FD);
-
-  // If the `await_suspend()` function is marked as `always_inline` explicitly,
-  // we should give the user the right to control the codegen.
-  if (FD->hasAttr() || FD->hasAttr())
-return;
-
-  // This is problematic if the user calls the await_suspend standalone. But on
-  // the on hand, it is not incorrect semantically since inlining is not part
-  // of the standard. On the other hand, it is relatively rare to call
-  // the await_suspend function standalone.
-  //
-  // And given we've already had the long-term plan, the current workaround
-  // looks relatively tolerant.
-  FD->addAttr(
-  NoInlineAttr::CreateImplicit(S.getASTContext(), FD->getLocation()));
+  return S.MaybeCreateExprWithCleanups(JustAddress);

ChuanqiXu9 wrote:

We also need to update the comments of the function after this patch. Also I 
feel like the above fixme can be removed.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -167,6 +167,47 @@ class CoroCloner {
 
 } // end anonymous namespace
 
+// FIXME:
+// Lower the intrinisc earlier if coroutine frame doesn't escape

ChuanqiXu9 wrote:

```suggestion
// Lower the intrinisc in CoroEarly phase if coroutine frame doesn't escape
```

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -232,16 +250,74 @@ static LValueOrRValue 
emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
   auto *NullPtr = llvm::ConstantPointerNull::get(CGF.CGM.Int8PtrTy);
   auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr});
 
+  const auto AwaitSuspendCanThrow =
+  AwaitSuspendStmtCanThrow(S.getSuspendExpr());
+
+  auto SuspendWrapper = CodeGenFunction(CGF.CGM).generateAwaitSuspendWrapper(
+  CGF.CurFn->getName(), Prefix, S);
+
+  llvm::CallBase *SuspendRet = nullptr;
+
   CGF.CurCoro.InSuspendBlock = true;
-  auto *SuspendRet = CGF.EmitScalarExpr(S.getSuspendExpr());
+
+  assert(CGF.CurCoro.Data && CGF.CurCoro.Data->CoroBegin &&
+ "expected to be called in coroutine context");
+
+  SmallVector SuspendIntrinsicCallArgs;
+  SuspendIntrinsicCallArgs.push_back(
+  CGF.getOrCreateOpaqueLValueMapping(S.getOpaqueValue()).getPointer(CGF));
+
+  SuspendIntrinsicCallArgs.push_back(CGF.CurCoro.Data->CoroBegin);
+  SuspendIntrinsicCallArgs.push_back(SuspendWrapper);
+
+  const auto SuspendReturnType = S.getSuspendReturnType();
+  llvm::Intrinsic::ID IID;
+
+  switch (SuspendReturnType) {
+  case CoroutineSuspendExpr::SuspendVoid:
+IID = llvm::Intrinsic::coro_await_suspend_void;
+break;
+  case CoroutineSuspendExpr::SuspendBool:
+IID = llvm::Intrinsic::coro_await_suspend_bool;
+break;
+  case CoroutineSuspendExpr::SuspendHandle:
+IID = llvm::Intrinsic::coro_await_suspend_handle;
+break;
+  }
+
+  llvm::Function *AwaitSuspendIntrinsic = CGF.CGM.getIntrinsic(IID);
+  // FIXME: add call attributes?
+  if (AwaitSuspendCanThrow)
+SuspendRet =
+CGF.EmitCallOrInvoke(AwaitSuspendIntrinsic, SuspendIntrinsicCallArgs);
+  else
+SuspendRet = CGF.EmitNounwindRuntimeCall(AwaitSuspendIntrinsic,
+ SuspendIntrinsicCallArgs);

ChuanqiXu9 wrote:

Do we really need to do this? I feel `EmitCallOrInvoke` is sufficient. LLVM is 
able to propagate `nounwind` in trivial cases.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -338,6 +414,71 @@ static QualType getCoroutineSuspendExprReturnType(const 
ASTContext &Ctx,
 }
 #endif
 
+llvm::Function *
+CodeGenFunction::generateAwaitSuspendWrapper(Twine const &CoroName,
+ Twine const &SuspendPointName,
+ CoroutineSuspendExpr const &S) {
+  std::string FuncName = "__await_suspend_wrapper_";
+  FuncName += CoroName.str();
+  FuncName += '_';
+  FuncName += SuspendPointName.str();

ChuanqiXu9 wrote:

I feel it is better to concat the name of the awaiter instead of the coroutines 
and suspend number.

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -232,16 +250,74 @@ static LValueOrRValue 
emitSuspendExpression(CodeGenFunction &CGF, CGCoroData &Co
   auto *NullPtr = llvm::ConstantPointerNull::get(CGF.CGM.Int8PtrTy);
   auto *SaveCall = Builder.CreateCall(CoroSave, {NullPtr});
 
+  const auto AwaitSuspendCanThrow =
+  AwaitSuspendStmtCanThrow(S.getSuspendExpr());
+
+  auto SuspendWrapper = CodeGenFunction(CGF.CGM).generateAwaitSuspendWrapper(
+  CGF.CurFn->getName(), Prefix, S);
+
+  llvm::CallBase *SuspendRet = nullptr;
+
   CGF.CurCoro.InSuspendBlock = true;
-  auto *SuspendRet = CGF.EmitScalarExpr(S.getSuspendExpr());
+
+  assert(CGF.CurCoro.Data && CGF.CurCoro.Data->CoroBegin &&
+ "expected to be called in coroutine context");
+
+  SmallVector SuspendIntrinsicCallArgs;
+  SuspendIntrinsicCallArgs.push_back(
+  CGF.getOrCreateOpaqueLValueMapping(S.getOpaqueValue()).getPointer(CGF));
+
+  SuspendIntrinsicCallArgs.push_back(CGF.CurCoro.Data->CoroBegin);
+  SuspendIntrinsicCallArgs.push_back(SuspendWrapper);
+
+  const auto SuspendReturnType = S.getSuspendReturnType();
+  llvm::Intrinsic::ID IID;

ChuanqiXu9 wrote:

```suggestion
  llvm::Intrinsic::ID AwaitSuspendIID;
```

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

@@ -173,19 +173,36 @@ static bool ResumeStmtCanThrow(const Stmt *S) {
   return false;
 }
 
+static bool AwaitSuspendStmtCanThrow(const Stmt *S) {
+  return ResumeStmtCanThrow(S);
+}
+
 // Emit suspend expression which roughly looks like:
 //
 //   auto && x = CommonExpr();
 //   if (!x.await_ready()) {
 //  llvm_coro_save();
-//  x.await_suspend(...); (*)
+//  llvm_coro_await_suspend(&x, frame, wrapper) (*)
 //  llvm_coro_suspend(); (**)
 //   }
 //   x.await_resume();
 //
 // where the result of the entire expression is the result of x.await_resume()
 //
-//   (*) If x.await_suspend return type is bool, it allows to veto a suspend:
+//   (*) llvm_coro_await_suspend_{void, bool, handle} is lowered to
+//  wrapper(&x, frame) when it's certain not to interfere with
+//  coroutine transform. await_suspend expression is
+//  asynchronous to the coroutine body and not all analyses
+//  and transformations can handle it correctly at the moment.
+//
+//  Wrapper function encapsulates x.await_suspend(...) call and looks like:
+//
+//  auto __await_suspend_wrapper(auto& awaiter, void* frame) {
+//std::coroutine_handle<> handle(frame);
+//return awaiter.await_suspend(handle);
+//  }
+//
+//  If x.await_suspend return type is bool, it allows to veto a suspend:

ChuanqiXu9 wrote:

```suggestion
//(**) If x.await_suspend return type is bool, it allows to veto a suspend:
```

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

[clang] [llvm] [coroutine] Implement llvm.coro.await.suspend intrinsic (PR #79712)

[Chuanqi Xu via cfe-commits]

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

[clang] [docs] [C++20] [Modules] Ideas for transitioning to modules (PR #80687)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

I'd like to land this in 2 weeks if no more comments come in.

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

[clang] [clangd] Fix C++20 modules crash (PR #81919)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 requested changes to this pull request.

This is not wanted. While we have a policy to revert patches if the patches 
break existing codes, the C++20 modules support in clangd is literally broken: 
https://github.com/clangd/clangd/issues/1293

Also the ability to skip ODR checks is an important feature to modules. So I 
don't think we should revert this now.

Also I don't understand why it can make clangd to crash. A test will be helpful 
for us to understand what happened.

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

[clang-tools-extra] [clangd] [C++20] [Modules] Introduce initial support for C++20 Modules (PR #66462)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

@sam-mccall ping~. Let's see if we can make it in clang19.

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

[clang] [Clang] CGCoroutine: Skip moving parameters if the allocation decision is false (PR #81195)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

yes if I remember correctly. I am away from the computer so I can't find the 
wording. 

But I don't think this is related to how the frame gets allocated. 

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

[clang] [Clang] CGCoroutine: Skip moving parameters if the allocation decision is false (PR #81195)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

I feel whether the param is needed is not related whether the allocation 
happens. 

The param isn't needed means it is not used. 

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

[clang] [docs] [C++20] [Modules] Ideas for transitioning to modules (PR #80687)

[Chuanqi Xu via cfe-commits]

@@ -610,6 +610,345 @@ the following style significantly:
 
 The key part of the tip is to reduce the duplications from the text includes.
 
+Ideas for converting to modules
+---
+
+For new libraries, we encourage them to use modules completely from day one if 
possible.
+This will be pretty helpful to make the whole ecosystems to get ready.
+
+For many existing libraries, it may be a breaking change to refactor themselves
+into modules completely. So that many existing libraries need to provide 
headers and module
+interfaces for a while to not break existing users.
+Here we provide some ideas to ease the transition process for existing 
libraries.
+**Note that the this section is only about helping ideas instead of 
requirement from clang**.
+
+Let's start with the case that there is no dependency or no dependent 
libraries providing
+modules for your library.
+
+ABI non-breaking styles
+~~~
+
+export-using style
+^^
+
+.. code-block:: c++
+
+  module;
+  #include "header_1.h"
+  #include "header_2.h"
+  ...
+  #include "header_n.h"
+  export module your_library;
+  export namespace your_namespace {
+using decl_1;
+using decl_2;
+...
+using decl_n;
+  }
+
+As the example shows, you need to include all the headers containing 
declarations needs
+to be exported and `using` such declarations in an `export` block. Then, 
basically,
+we're done.
+
+export extern-C++ style
+^^^
+
+.. code-block:: c++
+
+  module;
+  #include "third_party/A/headers.h"
+  #include "third_party/B/headers.h"
+  ...
+  #include "third_party/Z/headers.h"
+  export module your_library;
+  #define IN_MODULE_INTERFACE
+  extern "C++" {
+#include "header_1.h"
+#include "header_2.h"
+...
+#include "header_n.h"
+  }
+
+Then in your headers (from ``header_1.h`` to ``header_n.h``), you need to 
define the macro:
+
+.. code-block:: c++
+
+  #ifdef IN_MODULE_INTERFACE
+  #define EXPORT export
+  #else
+  #define EXPORT
+  #endif
+
+And you should put ``EXPORT`` to the beginning of the declarations you want to 
export.
+
+Also it is suggested to refactor your headers to include thirdparty headers 
conditionally:
+
+.. code-block:: c++
+
+  + #ifndef IN_MODULE_INTERFACE
+  #include "third_party/A/headers.h"
+  + #endif
+
+  #include "header_x.h"
+
+  ...
+
+This may be helpful to get better diagnostic messages if you forgot to update 
your module 
+interface unit file during maintaining.
+
+The reasoning for the practice is that the declarations in the language 
linkage are considered
+to be attached to the global module. So the ABI of your library in the modular 
version
+wouldn't change.
+
+While this style looks not as convenient as the export-using style, it is 
easier to convert 
+to other styles.
+
+ABI breaking style
+~~
+
+The term ``ABI breaking`` sounds terrifying generally. But you may want it 
here if you want
+to force your users to introduce your library in a consistent way. E.g., they 
either include
+your headers all the way or import your modules all the way.
+The style prevents the users to include your headers and import your modules 
at the same time
+in the same repo.

ChuanqiXu9 wrote:

For example, if the library is in a closed-ended ecosystem (e.g., only 
available to an organization internally), then  we want to provide modules 
interfaces for the library and we want our users to avoid the accidental 
performance cost due to mixing use of include and import, then such pattern 
helps. In case, the user repo A depends on a repo B and repo B include the 
modularized library, the repo A can reach out repo B to refactor itself. I feel 
such workflow can be understandable in a close ended organization.

 It will bring some burden to users initially, but in the end it would bring 
better quality. I understand this is not a helpful universally. But I feel it 
may be helpful to people who want to bring stronger requirement to the usage of 
their codes.

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

[clang] [docs] [C++20] [Modules] Ideas for transitioning to modules (PR #80687)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/80687

>From 2fcbdb034613ea6fdea4ce9efd5656116edb2ca1 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Mon, 5 Feb 2024 22:31:19 +0800
Subject: [PATCH] [docs] [C++20] [Modules] Ideas for transiting to modules

---
 clang/docs/StandardCPlusPlusModules.rst | 339 
 1 file changed, 339 insertions(+)

diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index c322805d8db5b..f4c3c4f0d8813 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -610,6 +610,345 @@ the following style significantly:
 
 The key part of the tip is to reduce the duplications from the text includes.
 
+Ideas for converting to modules
+---
+
+For new libraries, we encourage them to use modules completely from day one if 
possible.
+This will be pretty helpful to make the whole ecosystems to get ready.
+
+For many existing libraries, it may be a breaking change to refactor themselves
+into modules completely. So that many existing libraries need to provide 
headers and module
+interfaces for a while to not break existing users.
+Here we provide some ideas to ease the transition process for existing 
libraries.
+**Note that the this section is only about helping ideas instead of 
requirement from clang**.
+
+Let's start with the case that there is no dependency or no dependent 
libraries providing
+modules for your library.
+
+ABI non-breaking styles
+~~~
+
+export-using style
+^^
+
+.. code-block:: c++
+
+  module;
+  #include "header_1.h"
+  #include "header_2.h"
+  ...
+  #include "header_n.h"
+  export module your_library;
+  export namespace your_namespace {
+using decl_1;
+using decl_2;
+...
+using decl_n;
+  }
+
+As the example shows, you need to include all the headers containing 
declarations needs
+to be exported and `using` such declarations in an `export` block. Then, 
basically,
+we're done.
+
+export extern-C++ style
+^^^
+
+.. code-block:: c++
+
+  module;
+  #include "third_party/A/headers.h"
+  #include "third_party/B/headers.h"
+  ...
+  #include "third_party/Z/headers.h"
+  export module your_library;
+  #define IN_MODULE_INTERFACE
+  extern "C++" {
+#include "header_1.h"
+#include "header_2.h"
+...
+#include "header_n.h"
+  }
+
+Then in your headers (from ``header_1.h`` to ``header_n.h``), you need to 
define the macro:
+
+.. code-block:: c++
+
+  #ifdef IN_MODULE_INTERFACE
+  #define EXPORT export
+  #else
+  #define EXPORT
+  #endif
+
+And you should put ``EXPORT`` to the beginning of the declarations you want to 
export.
+
+Also it is suggested to refactor your headers to include thirdparty headers 
conditionally:
+
+.. code-block:: c++
+
+  + #ifndef IN_MODULE_INTERFACE
+  #include "third_party/A/headers.h"
+  + #endif
+
+  #include "header_x.h"
+
+  ...
+
+This may be helpful to get better diagnostic messages if you forgot to update 
your module 
+interface unit file during maintaining.
+
+The reasoning for the practice is that the declarations in the language 
linkage are considered
+to be attached to the global module. So the ABI of your library in the modular 
version
+wouldn't change.
+
+While this style looks not as convenient as the export-using style, it is 
easier to convert 
+to other styles.
+
+ABI breaking style
+~~
+
+The term ``ABI breaking`` sounds terrifying generally. But you may want it 
here if you want
+to force your users to introduce your library in a consistent way. E.g., they 
either include
+your headers all the way or import your modules all the way.
+The style prevents the users to include your headers and import your modules 
at the same time
+in the same repo.
+
+The pattern for ABI breaking style is similar with export extern-C++ style.
+
+.. code-block:: c++
+
+  module;
+  #include "third_party/A/headers.h"
+  #include "third_party/B/headers.h"
+  ...
+  #include "third_party/Z/headers.h"
+  export module your_library;
+  #define IN_MODULE_INTERFACE
+  #include "header_1.h"
+  #include "header_2.h"
+  ...
+  #include "header_n.h"
+
+  #if the number of .cpp files in your project are small
+  module :private;
+  #include "source_1.cpp"
+  #include "source_2.cpp"
+  ...
+  #include "source_n.cpp"
+  #else // the number of .cpp files in your project are a lot
+  // Using all the declarations from thirdparty libraries which are
+  // used in the .cpp files.
+  namespace third_party_namespace {
+using third_party_decl_used_in_cpp_1;
+using third_party_decl_used_in_cpp_2;
+...
+using third_party_decl_used_in_cpp_n;
+  }
+  #endif
+
+(And add `EXPORT` and conditional include to the headers as suggested in the 
export
+extern-C++ style section)
+
+Remember that the ABI get changed and we need to compile our source files into 
the
+new ABI format. This is t

[clang] [docs] [C++20] [Modules] Ideas for transitioning to modules (PR #80687)

[Chuanqi Xu via cfe-commits]

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

[clang] [docs] [C++20] [Modules] Ideas for transitioning to modules (PR #80687)

[Chuanqi Xu via cfe-commits]

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

[clang] [docs] [C++20] [Modules] Ideas for transforming to modules (PR #80687)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/80687

>From 7a9fb425a7dfb6429af969ec741c23c1e577e5fa Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Mon, 5 Feb 2024 22:31:19 +0800
Subject: [PATCH] [docs] [C++20] [Modules] Ideas for transiting to modules

---
 clang/docs/StandardCPlusPlusModules.rst | 339 
 1 file changed, 339 insertions(+)

diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index c322805d8db5b..e29aa73bc00af 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -610,6 +610,345 @@ the following style significantly:
 
 The key part of the tip is to reduce the duplications from the text includes.
 
+Ideas for converting to modules
+---
+
+For new libraries, we encourage them to use modules completely from day one if 
possible.
+This will be pretty helpful to make the whole ecosystems to get ready.
+
+For many existing libraries, it may be a breaking change to refactor themselves
+into modules completely. So that many existing libraries need to provide 
headers and module
+interfaces for a while to not break existing users.
+Here we provide some ideas to ease the transition process for existing 
libraries.
+**Note that the this section is only about helping ideas instead of 
requirement from clang**.
+
+Let's start with the case that there is no dependency or no dependent 
libraries providing
+modules for your library.
+
+ABI non-breaking styles
+~~~
+
+export-using style
+^^
+
+.. code-block:: c++
+
+  module;
+  #include "header_1.h"
+  #include "header_2.h"
+  ...
+  #include "header_n.h"
+  export module your_library;
+  export namespace your_namespace {
+using decl_1;
+using decl_2;
+...
+using decl_n;
+  }
+
+As the example shows, you need to include all the headers containing 
declarations needs
+to be exported and `using` such declarations in an `export` block. Then, 
basically,
+we're done.
+
+export extern-C++ style
+^^^
+
+.. code-block:: c++
+
+  module;
+  #include "third_party/A/headers.h"
+  #include "third_party/B/headers.h"
+  ...
+  #include "third_party/Z/headers.h"
+  export module your_library;
+  #define IN_MODULE_INTERFACE
+  extern "C++" {
+#include "header_1.h"
+#include "header_2.h"
+...
+#include "header_n.h"
+  }
+
+Then in your headers (from ``header_1.h`` to ``header_n.h``), you need to 
define the macro:
+
+.. code-block:: c++
+
+  #ifdef IN_MODULE_INTERFACE
+  #define EXPORT export
+  #else
+  #define EXPORT
+  #endif
+
+And you should put ``EXPORT`` to the beginning of the declarations you want to 
export.
+
+Also it is suggested to refactor your headers to include thirdparty headers 
conditionally:
+
+.. code-block:: c++
+
+  + #ifndef IN_MODULE_INTERFACE
+  #include "third_party/A/headers.h"
+  + #endif
+
+  #include "header_x.h"
+
+  ...
+
+This may be helpful to get better diagnostic messages if you forgot to update 
your module 
+interface unit file during maintaining.
+
+The reasoning for the practice is that the declarations in the language 
linkage are considered
+to be attached to the global module. So the ABI of your library in the modular 
version
+wouldn't change.
+
+While this style looks not as convenient as the export-using style, it is 
easier to convert 
+to other styles.
+
+ABI breaking style
+~~
+
+The term ``ABI breaking`` sounds terrifying generally. But you may want it 
here if you want
+to force your users to introduce your library in a consistent way. E.g., they 
either include
+your headers all the way or import your modules all the way.
+The style prevents the users to include your headers and import your modules 
at the same time
+in the same repo.
+
+The pattern for ABI breaking style is similar with export extern-C++ style.
+
+.. code-block:: c++
+
+  module;
+  #include "third_party/A/headers.h"
+  #include "third_party/B/headers.h"
+  ...
+  #include "third_party/Z/headers.h"
+  export module your_library;
+  #define IN_MODULE_INTERFACE
+  #include "header_1.h"
+  #include "header_2.h"
+  ...
+  #include "header_n.h"
+
+  #if the number of .cpp files in your project are small
+  module :private;
+  #include "source_1.cpp"
+  #include "source_2.cpp"
+  ...
+  #include "source_n.cpp"
+  #else // the number of .cpp files in your project are a lot
+  // Using all the declarations from thirdparty libraries which are
+  // used in the .cpp files.
+  namespace third_party_namespace {
+using third_party_decl_used_in_cpp_1;
+using third_party_decl_used_in_cpp_2;
+...
+using third_party_decl_used_in_cpp_n;
+  }
+  #endif
+
+(And add `EXPORT` and conditional include to the headers as suggested in the 
export
+extern-C++ style section)
+
+Remember that the ABI get changed and we need to compile our source files into 
the
+new ABI format. This is t

[clang] [docs] [C++20] [Modules] Ideas for transforming to modules (PR #80687)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 created 
https://github.com/llvm/llvm-project/pull/80687

This patch tries to provide some ideas to transform an existing libraries to 
modules. I feel this is helpful for users who is interested in modules from my 
observation.  While the syntax of modules look easy to understand, the practice 
gets harder if the users want to make compatible work with headers. Especially 
the `std` module can be installed in clang18, I think such document may be 
helpful.

I tried to not be too wordy in this document and I don't want the users to have 
impressions that they have to follow this or this is the best practice. So I 
tried to use the term `idea`.

Although I add some regular reviewers for modules, review opinions from users 
are highly recommended.

I want to land this before the releasing of clang18 (in the early March). We 
can and should improve this continuously.



>From 404713c8a359a3fbbf1bbc9a5e2bece75f74d464 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Mon, 5 Feb 2024 22:31:19 +0800
Subject: [PATCH] [docs] [C++20] [Modules] Ideas for transiting to modules

---
 clang/docs/StandardCPlusPlusModules.rst | 338 
 1 file changed, 338 insertions(+)

diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index c322805d8db5b..97170e92b8e09 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -610,6 +610,344 @@ the following style significantly:
 
 The key part of the tip is to reduce the duplications from the text includes.
 
+Ideas for converting to modules
+---
+
+For new libraries, we encourage them to use modules completely from day one if 
possible and.
+This will be pretty helpful to make the whole ecosystems to get ready.
+
+For many existing libraries, it may be a breaking change to refactor themselves
+into modules completely. So that many existing libraries need to provide 
headers and module
+interfaces for a while to not breaking existing users.
+Here we provide some ideas to ease the transition process for existing 
libraries.
+**Note that the this section is only about helping ideas instead of 
requirement from clang**.
+
+Let's start with the case that there is no dependency or no dependent 
libraries providing
+modules for you library.
+
+ABI non-breaking styles
+~~~
+
+export-using style
+^^
+
+.. code-block:: c++
+
+  module;
+  #include "header_1.h"
+  #include "header_2.h"
+  ...
+  #include "header_n.h"
+  export module your_library;
+  export namespace your_namespace {
+using decl_1;
+using decl_2;
+...
+using decl_n;
+  }
+
+As the example shows, you need to include all the headers containing 
declarations needs
+to be exported and `using` such declarations in an `export` block. Then, 
basically,
+we're done.
+
+export extern-C++ style
+^^^
+
+.. code-block:: c++
+
+  module;
+  #include "third_party/A/headers.h"
+  #include "third_party/B/headers.h"
+  ...
+  #include "third_party/Z/headers.h"
+  export module your_library;
+  #define IN_MODULE_INTERFACE
+  extern "C++" {
+#include "header_1.h"
+#include "header_2.h"
+...
+#include "header_n.h"
+  }
+
+Then in your headers (from ``header_1.h`` to ``header_n.h``), you need to 
define the macro:
+
+.. code-block:: c++
+
+  #ifdef IN_MODULE_INTERFACE
+  #define EXPORT export
+  #else
+  #define EXPORT
+  #endif
+
+And you should put ``EXPORT`` to the beginning of the declarations you want to 
export.
+
+Also it is suggested to refactor your headers to include thirdparty headers 
conditionally:
+
+.. code-block:: c++
+
+  + #ifndef IN_MODULE_INTERFACE
+  #include "third_party/A/headers.h"
+  + #endif
+
+  #include "header_x.h"
+
+  ...
+
+This may be helpful to get better diagnostic messages if you forgot to update 
your module 
+interface unit file during maintaining.
+
+The secret for the practice is that the declarations in the language linkage 
are considered
+to be attached to the global module. So the ABI of your library in the modular 
version
+wouldn't change.
+
+While this style looks not as convenient as the export-using style, it is 
easier to convert 
+to other styles.
+
+ABI breaking style
+~~
+
+The term ``ABI breaking`` sounds terrifying generally. But you may want it 
here if you want
+to force your users to introduce your library in a consistent way. E.g., they 
either include
+your headers all the way or they import your modules all the way.
+The style prevents the users to include your headers and import your modules 
in the same repo.
+
+The pattern for ABI breaking style is similar with export extern-C++ style.
+
+.. code-block:: c++
+
+  module;
+  #include "third_party/A/headers.h"
+  #include "third_party/B/headers.h"
+  ...
+  #include "third_party/Z/headers.h"
+  export module your_library;
+  #define IN_MODULE_INTERFACE
+  #include "heade

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;

ChuanqiXu9 wrote:

This is not resolved

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

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = baz::foo;
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl {{.*}} imported in A. {{.*}} baz
+// CHECK-NEXT: |-original Namespace {{.*}} 'baz'
+// CHECK-NEXT: |-TypeAliasDecl {{.*}} foo 'char'
+// CHECK-NEXT: | `-BuiltinType {{.*}} 'char'
+// CHECK-NEXT: |-UsingDecl {{.*}} baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace {{.*}} 'baz'
+// CHECK-NEXT: `-UsingShadowDecl 0x{{[^ ]*}} prev 0x[[SHADOW_ADDR:[^ ]*]] 
{{.*}} imported in A. {{.*}} 'foo'
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl {{.*}} baz
+// CHECK-NEXT: |-TypeAliasDecl {{.*}} foo 'char'
+// CHECK-NEXT: | `-BuiltinType {{.*}} 'char'
+// CHECK-NEXT: |-UsingDecl {{.*}} baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace {{.*}} 'baz'
+// CHECK-NEXT:  `-UsingShadowDecl 0x[[SHADOW_ADDR]] {{.*}} 'foo'

ChuanqiXu9 wrote:

While the RFC don't get consensus, I still don't like the test. There is only 
two `UsingShadowDecl` relevent, but we're checking too many things. I feel 
better if we can reduce the things we're checking or refactoring this into a 
unittest.

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

[clang] [Serialization] Load Specializations Lazily (PR #76774)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> This patch does too many things for me to be able to review it. This patch 
> fails on our infrastructure.
> 
> I'd propose to simplify it to basically D41416 + the on-disk hash table. We 
> should read all of the entries upon module loading to simplify the logic in 
> reading the hashes lazily. Reading the hashes lazily could be done but I 
> doubt its worth the complexity of the implementation at that stage.

It is surprising to me since the HashTable shouldn't be generated at all: 
https://github.com/llvm/llvm-project/pull/76774/files#diff-6fe53759e8d797c328c73ada5f3324c6248a8634ef36131c7eb2b9d89192bb64R4073-R4079

The suggestion to proceed sounds good to me  but it may take some time.

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

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> There is a lot of activity going on just in `clang/test/AST`, with multiple 
> commits a day. I think there is a time and place, and the right tradeoffs for 
> all kinds of tests we have. Shall we get others opinions? Shall we make an 
> RFC so we get the community aligned on this new direction?

Sounds not bad. I just sent 
https://discourse.llvm.org/t/rfc-prefer-unittests-over-matching-dumpped-ast/76729

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

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;

ChuanqiXu9 wrote:

ditto here.

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

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cppm -emit-module-interface -o 
%t/B.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/C.cpp -fmodule-file=A=%t/A.pcm 
-fmodule-file=B=%t/B.pcm -fsyntax-only -verify
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- bar.h
+class bar {
+  bar(baz::foo);
+};
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+#include "bar.h"
+export module B;

ChuanqiXu9 wrote:

```suggestion
export module B;
namespace baz {
export using foo;
}
export using ::bar;
```

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

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cppm -emit-module-interface -o 
%t/B.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/C.cpp -fmodule-file=A=%t/A.pcm 
-fmodule-file=B=%t/B.pcm -fsyntax-only -verify
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- bar.h
+class bar {
+  bar(baz::foo);
+};
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;

ChuanqiXu9 wrote:

```suggestion
export module A;
namespace baz {
export using foo;
}
```

Otherwise the declarations may be discarded once we implement 
http://eel.is/c++draft/module.global.frag#4

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

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

I still think the testing for dumped text is not acceptable. Let's make it 
prettier in unittest.

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

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> I took a look at what sort of complexity unittest would entail here. I don't 
> think it's a good compromise complexity wise, it's a lot of boilerplate just 
> to test a few AST nodes are correctly linked.

But they are much easier to maintain than dumpped AST text. I feel things get 
pretty clear with unittests. And I don't think there are a lot of complexities. 
They indeed have some boilerplates. But I think it is worhty to make things get 
tested. Also the boilerplates can be improved by refactoring and it is not so 
hurting since they are tests.

> 
> On the other hand, these AST tests don't look particularly out of place 
> compared to a lot of other AST tests we have.

But that is not good. We should try to avoid that.

> 
> However, I have seen that there are a lot of other related merging issues, so 
> I will leave the tangentially related tests for another MR.

I am not sure what's your meaning. What's your intention of testing of this 
patch itself? And I think it is not maintainable or acceptable to test the 
dumpped text in the following patches.



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

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

But the test for dumpped text still looks not good. Can we get rid of that?

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

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> The issue is up: #80252

Thanks. Although I feel it is too implementor's view, (I mean the end users can 
hardly understand it), it is much better.

> Yeah, looking at the similarities, there is a decent chance this is the same 
> issue, it's worth trying it out.

Yeah, let's give it a try.


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

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = baz::foo;
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl 0x[[BAZ_REDECL_ADDR:[^ ]*]] prev 0x[[BAZ_ADDR:[^ 
]*]] <{{.*}}> line:{{.*}} imported in A. hidden  baz
+// CHECK-NEXT: |-original Namespace 0x[[BAZ_ADDR]] 'baz'
+// CHECK-NEXT: |-TypeAliasDecl 0x[[ALIAS_REDECL_ADDR:[^ ]*]] prev 
0x[[ALIAS_ADDR:[^ ]*]] <{{.*}}> col:{{.*}} imported in A. hidden foo 
'char'
+// CHECK-NEXT: | `-BuiltinType 0x{{[^ ]*}} 'char'
+// CHECK-NEXT: |-UsingDecl 0x{{[^ ]*}} first 0x[[USING_ADDR:[^ ]*]] <{{.*}}> 
col:{{.*}} imported in A. hidden baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace 0x[[BAZ_REDECL_ADDR]] 'baz'
+// CHECK-NEXT: `-UsingShadowDecl 0x{{[^ ]*}} prev 0x[[SHADOW_ADDR:[^ ]*]] 
<{{.*}}> col:{{.*}} imported in A. hidden implicit TypeAlias 
0x[[ALIAS_REDECL_ADDR]] 'foo'

ChuanqiXu9 wrote:

In such case, I'd like to introduce unittest. For example 
https://github.com/llvm/llvm-project/blob/main/clang/unittests/Sema/SemaNoloadLookupTest.cpp,
 
https://github.com/llvm/llvm-project/blob/main/clang/unittests/Serialization/VarDeclConstantInitTest.cpp
 and 
https://github.com/llvm/llvm-project/pull/76774/files#diff-b1c26f074f25d8306b5ac4522770a2487a57194176355cf9af17688217573788.

While it may take you 10+ minutes, it will look much prettier than the current 
way.

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