[PATCH] D83652: Merge some of the PCH object support with modular codegen

[David Blaikie via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb198de67e0ba: Merge some of the PCH object support with 
modular codegen (authored by dblaikie).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652

Files:
  clang/include/clang/AST/ExternalASTSource.h
  clang/include/clang/Sema/MultiplexExternalSemaSource.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ModuleFile.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/MultiplexExternalSemaSource.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CodeGen/pch-dllexport.cpp

Index: clang/test/CodeGen/pch-dllexport.cpp
===
--- clang/test/CodeGen/pch-dllexport.cpp
+++ clang/test/CodeGen/pch-dllexport.cpp
@@ -3,13 +3,20 @@
 // RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCH %s
 
 // Build PCH with object file, then use it.
-// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-pch -building-pch-with-obj -o %t %s
-// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -building-pch-with-obj -o - %s | FileCheck -check-prefix=OBJ %s
-// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJ %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -fms-extensions -emit-pch -building-pch-with-obj -o %t %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -disable-llvm-optzns -fms-extensions -emit-obj -emit-llvm -include-pch %t -building-pch-with-obj -o - %s | FileCheck -check-prefix=OBJ %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -disable-llvm-optzns -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJ -check-prefix=PCHWITHOBJ-O1 %s
 
 // Check for vars separately to avoid having to reorder the check statements.
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -disable-llvm-optzns -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJVARS %s
+
+// Test the PCHWITHOBJ at -O0 where available_externally definitions are not
+// provided:
+// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-pch -building-pch-with-obj -o %t %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJ -check-prefix=PCHWITHOBJ-O0 %s
 // RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJVARS %s
 
+
 #ifndef IN_HEADER
 #define IN_HEADER
 
@@ -23,7 +30,8 @@
 inline void __declspec(dllexport) baz() {}
 // OBJ: define weak_odr dso_local dllexport void @"?baz@@YAXXZ"
 // PCH: define weak_odr dso_local dllexport void @"?baz@@YAXXZ"
-// PCHWITHOBJ: define weak_odr dso_local dllexport void @"?baz@@YAXXZ"
+// PCHWITHOBJ-O1: define available_externally dso_local void @"?baz@@YAXXZ"
+// PCHWITHOBJ-O0-NOT: define {{.*}}"?baz@@YAXXZ"
 
 
 struct __declspec(dllexport) S {
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1031,8 +1031,10 @@
   // that module interface unit, not by its users. (Inline variables are
   // still emitted in module users.)
   ModulesCodegen =
-  (Writer.WritingModule->Kind == Module::ModuleInterfaceUnit &&
-   Writer.Context->GetGVALinkageForVariable(D) == GVA_StrongExternal);
+  (Writer.WritingModule->Kind == Module::ModuleInterfaceUnit ||
+   (D->hasAttr() &&
+Writer.Context->getLangOpts().BuildingPCHWithObjectFile)) &&
+  Writer.Context->GetGVALinkageForVariable(D) == GVA_StrongExternal;
 }
 Record.push_back(ModulesCodegen);
 if (ModulesCodegen)
@@ -2469,7 +2471,10 @@
   Linkage = Writer->Context->GetGVALinkageForFunction(FD);
   ModulesCodegen = *Linkage == GVA_StrongExternal;
 }
-if (Writer->Context->getLangOpts().ModulesCodegen) {
+if (Writer->Context->getLangOpts().ModulesCodegen ||
+(FD->hasAttr() &&
+ Writer->Context->getLangOpts().BuildingPCHWithObjectFile)) {
+
   // Under -fmodules-codegen, codegen is performed for all non-internal,
   // non-always_inline functions, unless they are available elsewhere.
   if (!FD->hasAttr()) {
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1119,7 +1119,6 @@
   

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

@hans @llunak  - sounds like you're both fine with this? Either of you mind to 
give it a formal approval, if that's the case?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652



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

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

> Thanks for checking! So, in summary: This seems to work fine for Chromium, 
> but Chromium isn't really exercising this functionality (only insofar as the 
> functionality is enabled, but essentially a no-op)?

Yup.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652



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

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

> @llunak Would you be able to test this on anything you've got?

No, but thinking more about this, I think dllexport specifically voids the 
possible problems I listed. If I'm getting it right, dllexport is used only for 
code in the current library, so codegen won't create code referencing private 
symbols in other libraries, and dllexport means the code will be emitted 
anyway, so no unnecessary code generating. So I'm actually fine with the patch 
as it is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652



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

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D83652#2161924 , @hans wrote:

> In D83652#2159585 , @dblaikie wrote:
>
> > @hans - could you perhaps give me a quick summary of commands I could use 
> > to test this feature in Chromium (or anything else you might suggest) on a 
> > Linux box? I don't have a Windows machine, or any projects that use PCH. 
> > (or if you'd be willing to test this, that'd be great - see if the PCH 
> > object file has the same symbols before/after this patch)
>
>
> I tried to do this today, and then realized Chromium's PCH .obj files are 
> basically empty these days. Back when I did D48426 
> , dllexported inline functions were emitted 
> in the pch .obj file, but then Chromium started using /Zc:dllexportInlines- 
> which means we don't dllexport inlines anymore, and even if I remove that 
> flag we've moved to only including libc++ headers in our PCH, and nothing is 
> dllexport there.
>
> After applying and building with your patch, the PCH .obj files look the same.
>
> I also verified that building one of Chromium's test targets succeeds with 
> this patch.


Thanks for checking! So, in summary: This seems to work fine for Chromium, but 
Chromium isn't really exercising this functionality (only insofar as the 
functionality is enabled, but essentially a no-op)?

@llunak Would you be able to test this on anything you've got?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652



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

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D83652#2159585 , @dblaikie wrote:

> @hans - could you perhaps give me a quick summary of commands I could use to 
> test this feature in Chromium (or anything else you might suggest) on a Linux 
> box? I don't have a Windows machine, or any projects that use PCH. (or if 
> you'd be willing to test this, that'd be great - see if the PCH object file 
> has the same symbols before/after this patch)


I tried to do this today, and then realized Chromium's PCH .obj files are 
basically empty these days. Back when I did D48426 
, dllexported inline functions were emitted in 
the pch .obj file, but then Chromium started using /Zc:dllexportInlines- which 
means we don't dllexport inlines anymore, and even if I remove that flag we've 
moved to only including libc++ headers in our PCH, and nothing is dllexport 
there.

After applying and building with your patch, the PCH .obj files look the same.

I also verified that building one of Chromium's test targets succeeds with this 
patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652



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

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

@hans - could you perhaps give me a quick summary of commands I could use to 
test this feature in Chromium (or anything else you might suggest) on a Linux 
box? I don't have a Windows machine, or any projects that use PCH. (or if you'd 
be willing to test this, that'd be great - see if the PCH object file has the 
same symbols before/after this patch)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652



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

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

In D83652#2147539 , @dblaikie wrote:

> Not quite - it's intended to implement the D48426 
>  functionality using an implementation 
> strategy that is closer to modular code generation. Removing the need for the 
> module flag, using the MODULAR_CODEGEN_DECLS list, etc. But only putting the 
> dllexported entities in there (when using only building-pch-with-obj without 
> -fmodules-codegen).


I see. I still suggest testing this on a real codebase for the reasons outlined 
above.

> Yep, that was essentially what I was wondering - since you were proposing a 
> build mode/flags that would not be /Yc compatible, adding an explicit pch 
> build and pch object build step - was wondering if the build system support 
> for that was sufficiently available that it would be practical for users 
> (like Chromium) to migrate to that kind of model and no longer need /Yc 
> compatibility.

Not that I know for sure, but I personally doubt there's build system support 
for D83716 . The only build system I've tried 
is LibreOffice's custom gbuild, where I've added the support for 
-fpch-codegen/debuginfo myself, and even there's I'm going to stick with 
-building-pch-with-obj, since it's simpler to compile yet another .cxx file 
with an extra flag rather than have a build step with .pch as yet another kind 
of input file. But /Yc is presumably supported by pretty much anything on 
Windows.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652



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

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D83652#2146732 , @llunak wrote:

> The patch is incomplete, isn't it? It removes DeclIsFromPCHWithObjectFile(), 
> but it's still called from ASTContext::DeclMustBeEmitted(). The description 
> also mentions updating of the pch-codegen test, but that's not included.


Ah, right you are - arc diff only took the last commit on this branch. I've 
updated it/used arc correctly to get the whole branch worth of changes.

> But assuming this is intended to replace the D48426 
>  functionality with modular codegen,

Not quite - it's intended to implement the D48426 
 functionality using an implementation 
strategy that is closer to modular code generation. Removing the need for the 
module flag, using the MODULAR_CODEGEN_DECLS list, etc. But only putting the 
dllexported entities in there (when using only building-pch-with-obj without 
-fmodules-codegen).

The intent is not to change the behavior of D48426 
, but to simplify the implementation a bit by 
reducing the divergence between the two features.

>> Do either of you know if it'd be practical to move to something more similar 
>> to .pcm handling, where the pch itself is passed to the compilation, rather 
>> than homed as a side effect of compiling some other source file?
> 
> Do you mean dropping compatibility with 'cl.exe /Yc'?

Yep, that was essentially what I was wondering - since you were proposing a 
build mode/flags that would not be /Yc compatible, adding an explicit pch build 
and pch object build step - was wondering if the build system support for that 
was sufficiently available that it would be practical for users (like Chromium) 
to migrate to that kind of model and no longer need /Yc compatibility. That 
would further reduce some of the divergence between the two features (then it'd 
come down to only a "what entities do we want to home" question - the mechanics 
of homing them, building their object file, etc, would all be the same).

In D83652#2147097 , @hans wrote:

> In D83652#2146732 , @llunak wrote:
>
> > > Do either of you know if it'd be practical to move to something more 
> > > similar to .pcm handling, where the pch itself is passed to the 
> > > compilation, rather than homed as a side effect of compiling some other 
> > > source file?
> >
> > Do you mean dropping compatibility with 'cl.exe /Yc'?
>
>
> I don't think we can change the interface, because we want cl.exe 
> compatibility.


Fair enough - I guess the only other way to get the cl.exe behavior from the 
Clang driver using modular codegen-type functionality would be to use 4 steps:

- build PCH (with dllexport modular codegen)
- build PCH object file using PCH
- build source object using PCH
- ld -r the two objects together

But I guess the added driver complexity isn't worth the simplification inside 
Clang proper, probably. I guess we already have the "build PCH then use PCH to 
build object" part so /maybe/ it'd be feasible to extend it in this way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652



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

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 277449.
dblaikie added a comment.

Include all the commits


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652

Files:
  clang/include/clang/AST/ExternalASTSource.h
  clang/include/clang/Sema/MultiplexExternalSemaSource.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ModuleFile.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/MultiplexExternalSemaSource.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CodeGen/pch-dllexport.cpp

Index: clang/test/CodeGen/pch-dllexport.cpp
===
--- clang/test/CodeGen/pch-dllexport.cpp
+++ clang/test/CodeGen/pch-dllexport.cpp
@@ -3,13 +3,20 @@
 // RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCH %s
 
 // Build PCH with object file, then use it.
-// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-pch -building-pch-with-obj -o %t %s
-// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -building-pch-with-obj -o - %s | FileCheck -check-prefix=OBJ %s
-// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJ %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -fms-extensions -emit-pch -building-pch-with-obj -o %t %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -disable-llvm-optzns -fms-extensions -emit-obj -emit-llvm -include-pch %t -building-pch-with-obj -o - %s | FileCheck -check-prefix=OBJ %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -disable-llvm-optzns -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJ -check-prefix=PCHWITHOBJ-O1 %s
 
 // Check for vars separately to avoid having to reorder the check statements.
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -disable-llvm-optzns -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJVARS %s
+
+// Test the PCHWITHOBJ at -O0 where available_externally definitions are not
+// provided:
+// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-pch -building-pch-with-obj -o %t %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJ -check-prefix=PCHWITHOBJ-O0 %s
 // RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJVARS %s
 
+
 #ifndef IN_HEADER
 #define IN_HEADER
 
@@ -23,7 +30,8 @@
 inline void __declspec(dllexport) baz() {}
 // OBJ: define weak_odr dso_local dllexport void @"?baz@@YAXXZ"
 // PCH: define weak_odr dso_local dllexport void @"?baz@@YAXXZ"
-// PCHWITHOBJ: define weak_odr dso_local dllexport void @"?baz@@YAXXZ"
+// PCHWITHOBJ-O1: define available_externally dso_local void @"?baz@@YAXXZ"
+// PCHWITHOBJ-O0-NOT: define {{.*}}"?baz@@YAXXZ"
 
 
 struct __declspec(dllexport) S {
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1031,8 +1031,10 @@
   // that module interface unit, not by its users. (Inline variables are
   // still emitted in module users.)
   ModulesCodegen =
-  (Writer.WritingModule->Kind == Module::ModuleInterfaceUnit &&
-   Writer.Context->GetGVALinkageForVariable(D) == GVA_StrongExternal);
+  (Writer.WritingModule->Kind == Module::ModuleInterfaceUnit ||
+   (D->hasAttr() &&
+Writer.Context->getLangOpts().BuildingPCHWithObjectFile)) &&
+  Writer.Context->GetGVALinkageForVariable(D) == GVA_StrongExternal;
 }
 Record.push_back(ModulesCodegen);
 if (ModulesCodegen)
@@ -2469,7 +2471,10 @@
   Linkage = Writer->Context->GetGVALinkageForFunction(FD);
   ModulesCodegen = *Linkage == GVA_StrongExternal;
 }
-if (Writer->Context->getLangOpts().ModulesCodegen) {
+if (Writer->Context->getLangOpts().ModulesCodegen ||
+(FD->hasAttr() &&
+ Writer->Context->getLangOpts().BuildingPCHWithObjectFile)) {
+
   // Under -fmodules-codegen, codegen is performed for all non-internal,
   // non-always_inline functions, unless they are available elsewhere.
   if (!FD->hasAttr()) {
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1119,7 +1119,6 @@
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 16)); // Clang min.
   

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D83652#2146732 , @llunak wrote:

> > Do either of you know if it'd be practical to move to something more 
> > similar to .pcm handling, where the pch itself is passed to the 
> > compilation, rather than homed as a side effect of compiling some other 
> > source file?
>
> Do you mean dropping compatibility with 'cl.exe /Yc'?


I don't think we can change the interface, because we want cl.exe compatibility.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652



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

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[Luboš Luňák via Phabricator via cfe-commits]

llunak added a comment.

The patch is incomplete, isn't it? It removes DeclIsFromPCHWithObjectFile(), 
but it's still called from ASTContext::DeclMustBeEmitted(). The description 
also mentions updating of the pch-codegen test, but that's not included.

But assuming this is intended to replace the D48426 
 functionality with modular codegen, I 
mentioned already in D69778  (starting with 
'Mar 17 2020, 10:59 PM') that it's a question if this is possible. Modular 
codegen is a much bigger hammer than  D48426  
and it has also bigger possible problems: Possibly making compilation actually 
slower (even f63556d8b44b209e741833b0e6be3f8e72a1d91a mentions this), and 
possibly causing build problems because of referencing private symbols.  D48426 
 is much smaller both in gains and in problems 
(=safer).

> Do either of you know if it'd be practical to move to something more similar 
> to .pcm handling, where the pch itself is passed to the compilation, rather 
> than homed as a side effect of compiling some other source file?

Do you mean dropping compatibility with 'cl.exe /Yc'?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83652/new/

https://reviews.llvm.org/D83652



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

[PATCH] D83652: Merge some of the PCH object support with modular codegen

[David Blaikie via Phabricator via cfe-commits]

dblaikie created this revision.
dblaikie added reviewers: rnk, llunak, hans.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

I was trying to pick this up a bit when reviewing D48426 
 (& perhaps D69778 
) - in any case, looks like D48426 
 added a module level flag that might not be 
needed.

The D48426  implementation worked by setting a 
module level flag, then code generating contents from the PCH a special case in 
ASTContext::DeclMustBeEmitted would be used to delay emitting the definition of 
these functions if they came from a Module with this flag.

This strategy is similar to the one initially implemented for modular codegen 
that was removed in D29901  in favor of the 
modular decls list and a bit on each decl to specify whether it's homed to a 
module.

One major difference between PCH object support and modular code generation, 
other than the specific list of decls that are homed, is the compilation model: 
MSVC PCH modules are built into the object file for some other source file 
(when compiling that source file /Yc is specified to say "this compilation is 
where the PCH is homed"), whereas modular code generation invokes a separate 
compilation for the PCH alone. So the current modular code generation test of 
to decide if a decl should be emitted "is the module where this decl is 
serialized the current main file" has to be extended (as Lubos did in D69778 
) to also test the command line flag 
-building-pch-with-obj.

Otherwise the whole thing is basically streamlined down to the modular code 
generation path.

This even offers one extra material improvement compared to the existing 
divergent implementation: Homed functions are not emitted into object files 
that use the pch. Instead at -O0 they are not emitted into the IR at all, and 
at -O1 they are emitted using available_externally (existing functionality 
implemented for modular code generation). The pch-codegen test has been updated 
to reflect this new behavior.

[If possible: I'd love it if we could not have the extra MSVC-style way of 
accessing dllexport-pch-homing, and just do it the modular codegen way, but I 
understand that it might be a limitation of existing build systems. @hans / 
@thakis: Do either of you know if it'd be practical to move to something more 
similar to .pcm handling, where the pch itself is passed to the compilation, 
rather than homed as a side effect of compiling some other source file?]


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83652

Files:
  clang/include/clang/AST/ExternalASTSource.h
  clang/include/clang/Sema/MultiplexExternalSemaSource.h
  clang/include/clang/Serialization/ASTReader.h
  clang/include/clang/Serialization/ModuleFile.h
  clang/lib/Sema/MultiplexExternalSemaSource.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp

Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1119,7 +1119,6 @@
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 16)); // Clang min.
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Relocatable
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Timestamps
-  MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // PCHHasObjectFile
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Errors
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // SVN branch/tag
   unsigned MetadataAbbrevCode = Stream.EmitAbbrev(std::move(MetadataAbbrev));
@@ -1134,7 +1133,6 @@
 CLANG_VERSION_MINOR,
 !isysroot.empty(),
 IncludeTimestamps,
-false,
 ASTHasCompilerErrors};
 Stream.EmitRecordWithBlob(MetadataAbbrevCode, Record,
   getClangFullRepositoryVersion());
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2747,7 +2747,7 @@
 return VersionMismatch;
   }
 
-  bool hasErrors = Record[7];
+  bool hasErrors = Record[6];
   if (hasErrors && !DisableValidation && !AllowASTWithCompilerErrors) {
 Diag(diag::err_pch_with_compiler_errors);
 return HadErrors;
@@ -2765,8 +2765,6 @@
 
   F.HasTimestamps = Record[5];
 
-  F.PCHHasObjectFile = Record[6];
-
   const std::string  = getClangFullRepositoryVersion();
   StringRef ASTBranch = Blob;
   if (StringRef(CurBranch) != ASTBranch && !DisableValidation) {
@@ -8590,11 +8588,6 @@
   return getSubmodule(ID);
 }
 
-bool