[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D69585#1943222 , @llunak wrote:

> In D69585#1942431 , @rsmith wrote:
>
> > This needs to be done behind a flag. It's an explicit design goal that 
> > compilation behavior using a PCH or precompiled preamble behaves 
> > identically to compilation not using a PCH / precompiled preamble. The 
> > behavior of this patch is also non-conforming: we're only allowed to 
> > perform function template instantiations at their points of instantiation 
> > (essentially, at points of use + at the end of the translation unit).
>
>
> How is that different from -fmodules? Given that AFAICT this makes PCHs work 
> the same way as modules, this should mean -fmodules also both fails the 
> identical behaviour goal and is non-conforming.


Each header module ("header unit" in C++20 terminology) in a modules build is a 
distinct translation unit, so there's a point of instantiation at the end of 
it. It's also not a goal of `-fmodules` nor C++20 modules to produce identical 
output to a non-modules build; the modules language feature is intended to 
change the observable language semantics (unlike PCH, which is intended to 
improve build speed without affecting semantics).

> And to make sure I understand correctly, by behaving identically you mean 
> that all the compiler output should be identical without even benign 
> differences?

If by "benign differences" you mean things like different (but still correct) 
diagnostic output, then yes, that's the goal. I think producing the same 
diagnostics in a different order might be OK, though. For example, in an IDE, 
we can automatically precompile a header or a preamble to make interactive use 
more efficient, without changing the observable behavior of the compiler. We 
don't want a different diagnostic experience for interactive use versus 
standalone compilation.

In addition to the "explicit specialization after instantiation" problem you 
identified, attempting instantiation at the end of a preamble or PCH would also 
break things like this:

  template void f();
  struct X;
  void g() { f(); } // instantiation not performed yet
  
  template void f() { T t; };
  // -- end of preamble or PCH --
  struct X {};

Here, instantiation at the end of TU is valid, but instantiation at the end of 
preamble / PCH would reject this valid code, because we'd instantiate `f` 
while `X` is incomplete.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added a comment.

In D69585#1942431 , @rsmith wrote:

> This needs to be done behind a flag. It's an explicit design goal that 
> compilation behavior using a PCH or precompiled preamble behaves identically 
> to compilation not using a PCH / precompiled preamble. The behavior of this 
> patch is also non-conforming: we're only allowed to perform function template 
> instantiations at their points of instantiation (essentially, at points of 
> use + at the end of the translation unit).


How is that different from -fmodules? Given that AFAICT this makes PCHs work 
the same way as modules, this should mean -fmodules also both fails the 
identical behaviour goal and is non-conforming.

And to make sure I understand correctly, by behaving identically you mean that 
all the compiler output should be identical without even benign differences?


Repository:
  rC Clang

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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This needs to be done behind a flag. It's an explicit design goal that 
compilation behavior using a PCH or precompiled preamble behaves identically to 
compilation not using a PCH / precompiled preamble. The behavior of this patch 
is also non-conforming: we're only allowed to perform function template 
instantiations at their points of instantiation (essentially, at points of use 
+ at the end of the translation unit).

I'd be OK with either a (conforming compilation mode) flag that instructs Clang 
to attempt to perform instantiations at the point of use (assuming the template 
is defined at that point), or with a (non-conforming) flag that instructs Clang 
to perform instantiations at the end of the PCH, as it would when compiling a 
header unit.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added a comment.

Ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added a comment.

In D69585#1922108 , @dblaikie wrote:

> Does this still have an OpenMP special case? The production code changes 
> don't seem to mention OpenMP anymore, but there's still a lot of test updates 
> for OpenMP - what are they for?


It's been handled by b841b9e96e605bed5a1f9b846a07aae88c65ce02 
 . The 
tests need updates because they test compilation both with and without PCH use, 
and this patch reorders templates in the output, without any functional change.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Could you upload this with full context ( 
https://llvm.org/docs/Phabricator.html#id4 mentions how to do this)?

Does this still have an OpenMP special case? The production code changes don't 
seem to mention OpenMP anymore, but there's still a lot of test updates for 
OpenMP - what are they for?

@rsmith could you take a look at this - the pending instantiations stuff isn't 
something I'm sufficiently familiar with to feel comfortable signing off on 
this alone.


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

If the current listed reviewers on this patch are not the best people for 
reviewing this, would one of them please suggest a more appropriate reviewer so 
we can get some traction on this?


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added a comment.

Ping...


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added a comment.

Ping..


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added a comment.

Ping.


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added a comment.

Ping again. Is there still something more to do here?


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak marked 2 inline comments as done.
llunak added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:985-986
+
+// FIXME: Instantiating implicit templates already in the PCH breaks some
+// OpenMP-specific code paths, see https://reviews.llvm.org/D69585 .
+if(LangOpts.PCHInstantiateTemplates && !LangOpts.OpenMP) {

dblaikie wrote:
> llunak wrote:
> > rnk wrote:
> > > This really deserves to be debugged before we land it.
> > I debugged this more than 2 months ago, that's why the OpenMP code owner is 
> > added as a reviewer.  The initial diff (that I have no idea how to display 
> > here) included a suggested fix and the description said this was a WIP and 
> > included my findings about it. As far as I can tell the only effect that 
> > had was that this patch was sitting here all the time without a single 
> > reaction, so if nobody OpenMP related cares then I do not either.
> > 
> > 
> So the original comment said "breaks some Open-MP specific code paths" - what 
> kind of breakage occurs? (& I take it with the patch in its current form that 
> breakage is present in the patch (as opposed to the previous version that 
> conditionalized this feature so it didn't happen when OpenMP was enabled)?)
See https://reviews.llvm.org/D72759 . It's already been handled.




Comment at: clang/test/PCH/specialization-after-instantiation.cpp:2
+// Test this without pch.
+// RUN: %clang_cc1 -fsyntax-only -verify -DBODY %s
+

dblaikie wrote:
> Does this need testing without PCH here? Presumably that's already been 
> tested elsewhere for some time now?
Technically not, but it can't hurt (it seems to be the common practice with 
testing PCHs), and I'm not aware of that elsewhere place (i.e. I've looked and 
couldn't find it).



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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:985-986
+
+// FIXME: Instantiating implicit templates already in the PCH breaks some
+// OpenMP-specific code paths, see https://reviews.llvm.org/D69585 .
+if(LangOpts.PCHInstantiateTemplates && !LangOpts.OpenMP) {

llunak wrote:
> rnk wrote:
> > This really deserves to be debugged before we land it.
> I debugged this more than 2 months ago, that's why the OpenMP code owner is 
> added as a reviewer.  The initial diff (that I have no idea how to display 
> here) included a suggested fix and the description said this was a WIP and 
> included my findings about it. As far as I can tell the only effect that had 
> was that this patch was sitting here all the time without a single reaction, 
> so if nobody OpenMP related cares then I do not either.
> 
> 
So the original comment said "breaks some Open-MP specific code paths" - what 
kind of breakage occurs? (& I take it with the patch in its current form that 
breakage is present in the patch (as opposed to the previous version that 
conditionalized this feature so it didn't happen when OpenMP was enabled)?)



Comment at: clang/test/PCH/specialization-after-instantiation.cpp:2
+// Test this without pch.
+// RUN: %clang_cc1 -fsyntax-only -verify -DBODY %s
+

Does this need testing without PCH here? Presumably that's already been tested 
elsewhere for some time now?


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Adding a ping since it's been a week with no additional feedback for the author.


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:984
+
+PerformPendingInstantiations();
   }

aganea wrote:
> All tests pass if you add `if (LangOpts.BuildingPCHWithObjectFile)` here. But 
> if a specialization occurs inside a .CPP which includes the .PCH, not sure 
> what would/should happen, and if that'S handled. Definitely needs a test for 
> that. @rsmith WDYT?
> All tests pass if you add if (LangOpts.BuildingPCHWithObjectFile) here.

That is because BuildingPCHWithObjectFile is almost unused by tests, so this 
just disables the change. But the change should be enabled for everything, it 
doesn't depend on whether PCH's object file is used.

> But if a specialization occurs inside a .CPP which includes the .PCH, not 
> sure what would/should happen, and if that'S handled. 

You mean this?

```
a.h:
#pragma once
template< typename T >
struct A { int foo() const; };
int bar(A* a) { return a->foo(); }
a.cpp:
#include "a.h"
template<> int A::foo() const { return 10; }

```
That errors out with "explicit specialization of 'foo' after instantiation" 
with or without PCH used, with or without my patch. I don't know about tests 
for it though. I can add this as one.



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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:984
+
+PerformPendingInstantiations();
   }

All tests pass if you add `if (LangOpts.BuildingPCHWithObjectFile)` here. But 
if a specialization occurs inside a .CPP which includes the .PCH, not sure what 
would/should happen, and if that'S handled. Definitely needs a test for that. 
@rsmith WDYT?


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D69585#1825252 , @llunak wrote:

> In D69585#1825133 , @ABataev wrote:
>
> > I thought you were going to add an option or a flag to control the 
> > behavior? If so, just provide an option in tests to avoid triggering of the 
> > new behavior (except for declare_target... test and those 2 you modified 
> > already) and that's it.
>
>
> It's not included in the latest version of the patch. As written above, I'm 
> reasonably sure I was mistaken about the need for a flag, and it should be ok 
> to simply do the change unconditionally. I can put the flag back just for the 
> purpose of the tests if you want, that'd certainly make handling of the tests 
> trivial, but then the tests wouldn't really test "normal" PCHs, so do you 
> really want that?


Of course not. I was just wandering if you still going to use a flag. If you're 
not going to use it, then there is only one choice - update the tests. The 
tests are written so to test that emitting/including PCH does not break the 
codegen. So it should be tested.


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added a comment.

In D69585#1825133 , @ABataev wrote:

> I thought you were going to add an option or a flag to control the behavior? 
> If so, just provide an option in tests to avoid triggering of the new 
> behavior (except for declare_target... test and those 2 you modified already) 
> and that's it.


It's not included in the latest version of the patch. As written above, I'm 
reasonably sure I was mistaken about the need for a flag, and it should be ok 
to simply do the change unconditionally. I can put the flag back just for the 
purpose of the tests if you want, that'd certainly make handling of the tests 
trivial, but then the tests wouldn't really test "normal" PCHs, so do you 
really want that?


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D69585#1823969 , @llunak wrote:

> In D69585#1821831 , @aganea wrote:
>
> > What is the error?
>
>
> I take that part back, actually. I don't quite remember anymore what exactly 
> I did in October, but if I now revert the PCH tweaks in LibreOffice I did to 
> avoid the error, the compilation fails even without the patch or with GCC. So 
> I assume what really happened was that code changes triggered the error and I 
> incorrectly assumed it was because of my patch. So, unless proven otherwise, 
> I take it that my patch is actually technically correct without causing any 
> code regressions (the OpenMP problem has just been fixed).
>
> What remains is those 22 tests which fail because moving the instantiations 
> reorders the code that is expected by FileCheck. This patch handles 2 of 
> them, and it's already rather tedious (the OpenMP tests are large). Is this 
> really the way to handle them, or does somebody have a better idea?


I thought you were going to add an option or a flag to control the behavior? If 
so, just provide an option in tests to avoid triggering of the new behavior 
(except for declare_target... test and those 2 you modified already) and that's 
it.


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak updated this revision to Diff 238477.
llunak added a comment.

In D69585#1821831 , @aganea wrote:

> What is the error?


I take that part back, actually. I don't quite remember anymore what exactly I 
did in October, but if I now revert the PCH tweaks in LibreOffice I did to 
avoid the error, the compilation fails even without the patch or with GCC. So I 
assume what really happened was that code changes triggered the error and I 
incorrectly assumed it was because of my patch. So, unless proven otherwise, I 
take it that my patch is actually technically correct without causing any code 
regressions (the OpenMP problem has just been fixed).

What remains is those 22 tests which fail because moving the instantiations 
reorders the code that is expected by FileCheck. This patch handles 2 of them, 
and it's already rather tedious (the OpenMP tests are large). Is this really 
the way to handle them, or does somebody have a better idea?


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

https://reviews.llvm.org/D69585

Files:
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGenCXX/vla-lambda-capturing.cpp
  clang/test/OpenMP/single_codegen.cpp

Index: clang/test/OpenMP/single_codegen.cpp
===
--- clang/test/OpenMP/single_codegen.cpp
+++ clang/test/OpenMP/single_codegen.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 -verify -fopenmp -fnoopenmp-use-tls -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-llvm %s -fexceptions -fcxx-exceptions -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -fnoopenmp-use-tls -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-llvm %s -fexceptions -fcxx-exceptions -o - | FileCheck --check-prefixes=CHECK,NOPCH %s
 // RUN: %clang_cc1 -fopenmp -fnoopenmp-use-tls -x c++ -std=c++11 -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -fnoopenmp-use-tls -x c++ -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fnoopenmp-use-tls -x c++ -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck --check-prefixes=CHECK,PCH %s
 // RUN: %clang_cc1 -verify -triple x86_64-apple-darwin10 -std=c++11 -fopenmp -fnoopenmp-use-tls -fexceptions -fcxx-exceptions -debug-info-kind=line-tables-only -x c++ -emit-llvm %s -o - | FileCheck %s --check-prefix=TERM_DEBUG
 // RUN: %clang_cc1 -verify -fopenmp -fnoopenmp-use-tls -x c++ -std=c++11 -DARRAY -triple x86_64-apple-darwin10 -emit-llvm %s -o - | FileCheck -check-prefix=ARRAY %s
 
@@ -230,6 +230,58 @@
 // ARRAY: store %struct.St* %{{.+}}, %struct.St** %{{.+}},
 #endif
 
+// PCH-LABEL: @_ZN3SSTIdEC2Ev
+// PCH: getelementptr inbounds [[SST_TY]], [[SST_TY]]* %{{.+}}, i32 0, i32 0
+// PCH-NEXT: store double 0.00e+00, double* %
+// PCH-NEXT: getelementptr inbounds [[SST_TY]], [[SST_TY]]* %{{.+}}, i32 0, i32 0
+// PCH-NEXT: store double* %{{.+}}, double** %
+// PCH-NEXT: load double*, double** %
+// PCH-NEXT: load double, double* %
+// PCH-NEXT: bitcast i64* %{{.+}} to double*
+// PCH-NEXT: store double %{{.+}}, double* %
+// PCH-NEXT: load i64, i64* %
+// PCH-NEXT: call void ([[IDENT_T_TY]]*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call([[IDENT_T_TY]]* @{{.+}}, i32 2, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, [[SST_TY]]*, i64)* [[SST_MICROTASK:@.+]] to void
+// PCH-NEXT: ret void
+
+// PCH: define internal void [[SST_MICROTASK]](i32* {{[^,]+}}, i32* {{[^,]+}}, [[SST_TY]]* {{.+}}, i64 {{.+}})
+// PCH: [[RES:%.+]] = call i32 @__kmpc_single([[IDENT_T_TY]]* @{{.+}}, i32 %{{.+}})
+// PCH-NEXT: icmp ne i32 [[RES]], 0
+// PCH-NEXT: br i1
+
+// PCH: getelementptr inbounds %{{.+}}, %{{.+}}* %{{.+}}, i32 0, i32 1
+// PCH-NEXT: load double*, double** %
+// PCH-NEXT: store double* %
+// PCH-LABEL: invoke void @_ZZN3SSTIdEC1EvENKUlvE_clEv(
+
+// PCH: call void @__kmpc_end_single([[IDENT_T_TY]]* @{{.+}}, i32 %{{.+}})
+// PCH-NEXT: store i32 1, i32* [[DID_IT]],
+// PCH-NEXT: br label
+
+// PCH: call void @__kmpc_end_single([[IDENT_T_TY]]* @{{.+}}, i32 %{{.+}})
+// PCH-NEXT: br label
+
+// PCH: getelementptr inbounds [1 x i8*], [1 x i8*]* [[LIST:%.+]], i64 0, i64 0
+// PCH: load double*, double** %
+// PCH-NEXT: bitcast double* %
+// PCH-NEXT: store i8* %
+// PCH-NEXT: bitcast [1 x i8*]* [[LIST]] to i8*
+// PCH-NEXT: load i32, i32* [[DID_IT]],
+// PCH-NEXT: call void @__kmpc_copyprivate([[IDENT_T_TY]]* @{{.+}}, i32 %{{.+}}, i64 8, i8* %{{.+}}, void (i8*, i8*)* [[COPY_FUNC:@[^,]+]], i32 %{{.+}})
+// PCH-NEXT:  ret void
+
+// PCH-LABEL: @_ZZN3SSTIdEC1EvENKUlvE_clEv(
+// PCH: getelementptr inbounds %{{.+}}, %{{.+}}* %{{.+}}, i32 0, i32 1
+// PCH-NEXT: getelementptr inbounds %{{.+}}, %{{.+}}* %{{.+}}, i32 0, i32 1
+// PCH-NEXT: load double*, double** %
+// P

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

I'm glad you're fixing this, this problem has came up in our profile traces as 
well.

> The only way this breaks things that I've managed to find is if a .cpp file 
> using the PCH adds another template specialization that's not mentioned in 
> the PCH

What is the error?

Could you please add test(s)? We need a test especially for the error you're 
mentioning. It has to be clear to the user that an error message in the /Yu 
.obj (because of a specialization) in fact occurs because usage of 
`-pch-instantiate-templates` in the pch.obj.

Have you investigated how MSVC handles this? Using a PCH in a OBJ is a lot 
faster with MSVC, so I'm guessing they have some trick.




Comment at: clang/lib/Sema/Sema.cpp:987
+// OpenMP-specific code paths, see https://reviews.llvm.org/D69585 .
+if(LangOpts.PCHInstantiateTemplates && !LangOpts.OpenMP) {
+  llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",

clang-format please.



Comment at: clang/lib/Sema/Sema.cpp:988
+if(LangOpts.PCHInstantiateTemplates) {
+  llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
+ StringRef(""));

Why not move this time trace scope inside `PerformPendingInstantiations()`? 
(and the other one at L927 too)


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak updated this revision to Diff 238217.
llunak edited the summary of this revision.
llunak removed a project: OpenMP.
llunak added a comment.

In order to simplify this, I've updated the patch to remove any reference to 
OpenMP and I've moved that part to https://reviews.llvm.org/D72759 .

This means that the only thing missing here should be adding tests. Which, as 
said above, I'm unsure how to do exactly given that it'd probably need 
duplicating all PCH tests to use this option, so please advise how to do this 
so that the patch can be accepted.

In D69585#1820849 , @ABataev wrote:

> I don't see any crashes in OpenMP tests, other tests just must be updated by 
> the author, if this patch will ever going to be landed. I don't think it is a 
> good idea to have a not fully functional implementation, even guarded by the 
> option.


Please see https://reviews.llvm.org/D72759 .


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

https://reviews.llvm.org/D69585

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/Sema.cpp


Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -983,6 +983,12 @@
  LateParsedInstantiations.begin(),
  LateParsedInstantiations.end());
 LateParsedInstantiations.clear();
+
+if(LangOpts.PCHInstantiateTemplates) {
+  llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
+ StringRef(""));
+  PerformPendingInstantiations();
+}
   }
 
   DiagnoseUnterminatedPragmaPack();
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3294,6 +3294,7 @@
 
   Opts.CompleteMemberPointers = Args.hasArg(OPT_fcomplete_member_pointers);
   Opts.BuildingPCHWithObjectFile = Args.hasArg(OPT_building_pch_with_obj);
+  Opts.PCHInstantiateTemplates = Args.hasArg(OPT_pch_instantiate_templates);
 }
 
 static bool isStrictlyPreprocessorAction(frontend::ActionKind Action) {
Index: clang/include/clang/Driver/CC1Options.td
===
--- clang/include/clang/Driver/CC1Options.td
+++ clang/include/clang/Driver/CC1Options.td
@@ -676,6 +676,8 @@
   HelpText<"Disable inclusion of timestamp in precompiled headers">;
 def building_pch_with_obj : Flag<["-"], "building-pch-with-obj">,
   HelpText<"This compilation is part of building a PCH with corresponding 
object file.">;
+def pch_instantiate_templates : Flag<["-"], "pch-instantiate-templates">,
+  HelpText<"Perform pending template instantiations already while building the 
PCH.">;
 
 def aligned_alloc_unavailable : Flag<["-"], "faligned-alloc-unavailable">,
   HelpText<"Aligned allocation/deallocation functions are unavailable">;
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -161,6 +161,7 @@
 BENIGN_LANGOPT(CompilingPCH, 1, 0, "building a pch")
 BENIGN_LANGOPT(BuildingPCHWithObjectFile, 1, 0, "building a pch which has a 
corresponding object file")
 BENIGN_LANGOPT(CacheGeneratedPCH, 1, 0, "cache generated PCH files in memory")
+BENIGN_LANGOPT(PCHInstantiateTemplates, 1, 0, "performing pending template 
instantiations already while building a pch")
 COMPATIBLE_LANGOPT(ModulesDeclUse, 1, 0, "require declaration of module 
uses")
 BENIGN_LANGOPT(ModulesSearchAll  , 1, 1, "searching even non-imported modules 
to find unresolved references")
 COMPATIBLE_LANGOPT(ModulesStrictDeclUse, 1, 0, "requiring declaration of 
module uses and all headers to be in modules")


Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -983,6 +983,12 @@
  LateParsedInstantiations.begin(),
  LateParsedInstantiations.end());
 LateParsedInstantiations.clear();
+
+if(LangOpts.PCHInstantiateTemplates) {
+  llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
+ StringRef(""));
+  PerformPendingInstantiations();
+}
   }
 
   DiagnoseUnterminatedPragmaPack();
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3294,6 +3294,7 @@
 
   Opts.CompleteMemberPointers = Args.hasArg(OPT_fcomplete_member_pointers);
   Opts.BuildingPCHWithObjectFile = Ar

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

I don't see any crashes in OpenMP tests, other tests just must be updated by 
the author, if this patch will ever going to be landed. I don't think it is a 
good idea to have a not fully functional implementation, even guarded by the 
option.


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak marked 2 inline comments as done.
llunak added a comment.

In D69585#1818205 , @rnk wrote:

> I'm interested in making clang do this, but I think this needs significantly 
> more work until this is ready to land. It needs in-tree tests.


What tests specifically? I can add one test that just uses the option and then 
checks the PCH already contains what it should, but besides that this would 
need to repeat all PCH tests with this enabled. Should I just do that?

> I assumed we'd want to hook it up to `clang-cl /Yc` and `/Yu`.

If you mean automatically, then probably not. As said in the description, this 
leads to an error if a PCH'd template has a specialization that's not at least 
mentioned in the PCH. That's not a big deal and it's easy to handle, but it's 
still technically a regression. I myself wouldn't mind and would be fine with 
making this the default, but I assume (plural) you wouldn't.




Comment at: clang/include/clang/Basic/LangOptions.def:163
 BENIGN_LANGOPT(CacheGeneratedPCH, 1, 0, "cache generated PCH files in memory")
+BENIGN_LANGOPT(PCHInstantiateTemplates, 1, 0, "performing pending template 
instantiations already while building a pch")
 COMPATIBLE_LANGOPT(ModulesDeclUse, 1, 0, "require declaration of module 
uses")

rnk wrote:
> I think both sides of a PCH user and creator will need to agree on this, so 
> this isn't a "benign" langopt, is it? This is a regular one, and we should 
> diagnose mismatches between the PCH creation and use.
The flag needs to be used only when creating the PCH. Users don't care, they'll 
run the instantiation too, the flag will just make users do less repeated work.





Comment at: clang/lib/Sema/Sema.cpp:985-986
+
+// FIXME: Instantiating implicit templates already in the PCH breaks some
+// OpenMP-specific code paths, see https://reviews.llvm.org/D69585 .
+if(LangOpts.PCHInstantiateTemplates && !LangOpts.OpenMP) {

rnk wrote:
> This really deserves to be debugged before we land it.
I debugged this more than 2 months ago, that's why the OpenMP code owner is 
added as a reviewer.  The initial diff (that I have no idea how to display 
here) included a suggested fix and the description said this was a WIP and 
included my findings about it. As far as I can tell the only effect that had 
was that this patch was sitting here all the time without a single reaction, so 
if nobody OpenMP related cares then I do not either.




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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I'm interested in making clang do this, but I think this needs significantly 
more work until this is ready to land. It needs in-tree tests. I assumed we'd 
want to hook it up to `clang-cl /Yc` and `/Yu`.




Comment at: clang/include/clang/Basic/LangOptions.def:163
 BENIGN_LANGOPT(CacheGeneratedPCH, 1, 0, "cache generated PCH files in memory")
+BENIGN_LANGOPT(PCHInstantiateTemplates, 1, 0, "performing pending template 
instantiations already while building a pch")
 COMPATIBLE_LANGOPT(ModulesDeclUse, 1, 0, "require declaration of module 
uses")

I think both sides of a PCH user and creator will need to agree on this, so 
this isn't a "benign" langopt, is it? This is a regular one, and we should 
diagnose mismatches between the PCH creation and use.



Comment at: clang/lib/Sema/Sema.cpp:985-986
+
+// FIXME: Instantiating implicit templates already in the PCH breaks some
+// OpenMP-specific code paths, see https://reviews.llvm.org/D69585 .
+if(LangOpts.PCHInstantiateTemplates && !LangOpts.OpenMP) {

This really deserves to be debugged before we land it.


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added a comment.

Ping.


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak added a comment.

Ping.


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak updated this revision to Diff 229722.
llunak added a comment.

It turns out that this patch may introduce unwanted changes, specifically it 
can cause err_specialization_after_instantiation if the specialization is done 
in a source file but needed already by code in the PCH. But this seems to be 
rare (I've encountered it only after building the Skia library with 
put-everything-in-the-PCH), is easy to avoid (PCHs often need little tweaks 
anyway) and the performance gain is very much worth it.

Still, this means there should be an option to enable this. Done in this 
version of the patch. I don't know how to handle tests for it, pretty much all 
the tests pass as said above, but it seems a bit silly to duplicate all the 
PCH-related ones to also use the flag.


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

https://reviews.llvm.org/D69585

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/Sema.cpp


Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -981,6 +981,14 @@
  LateParsedInstantiations.begin(),
  LateParsedInstantiations.end());
 LateParsedInstantiations.clear();
+
+// FIXME: Instantiating implicit templates already in the PCH breaks some
+// OpenMP-specific code paths, see https://reviews.llvm.org/D69585 .
+if(LangOpts.PCHInstantiateTemplates && !LangOpts.OpenMP) {
+  llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
+ StringRef(""));
+  PerformPendingInstantiations();
+}
   }
 
   DiagnoseUnterminatedPragmaPack();
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3223,6 +3223,7 @@
 
   Opts.CompleteMemberPointers = Args.hasArg(OPT_fcomplete_member_pointers);
   Opts.BuildingPCHWithObjectFile = Args.hasArg(OPT_building_pch_with_obj);
+  Opts.PCHInstantiateTemplates = Args.hasArg(OPT_pch_instantiate_templates);
 }
 
 static bool isStrictlyPreprocessorAction(frontend::ActionKind Action) {
Index: clang/include/clang/Driver/CC1Options.td
===
--- clang/include/clang/Driver/CC1Options.td
+++ clang/include/clang/Driver/CC1Options.td
@@ -672,6 +672,8 @@
   HelpText<"Disable inclusion of timestamp in precompiled headers">;
 def building_pch_with_obj : Flag<["-"], "building-pch-with-obj">,
   HelpText<"This compilation is part of building a PCH with corresponding 
object file.">;
+def pch_instantiate_templates : Flag<["-"], "pch-instantiate-templates">,
+  HelpText<"Perform pending template instantiations already while building the 
PCH.">;
 
 def aligned_alloc_unavailable : Flag<["-"], "faligned-alloc-unavailable">,
   HelpText<"Aligned allocation/deallocation functions are unavailable">;
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -160,6 +160,7 @@
 BENIGN_LANGOPT(CompilingPCH, 1, 0, "building a pch")
 BENIGN_LANGOPT(BuildingPCHWithObjectFile, 1, 0, "building a pch which has a 
corresponding object file")
 BENIGN_LANGOPT(CacheGeneratedPCH, 1, 0, "cache generated PCH files in memory")
+BENIGN_LANGOPT(PCHInstantiateTemplates, 1, 0, "performing pending template 
instantiations already while building a pch")
 COMPATIBLE_LANGOPT(ModulesDeclUse, 1, 0, "require declaration of module 
uses")
 BENIGN_LANGOPT(ModulesSearchAll  , 1, 1, "searching even non-imported modules 
to find unresolved references")
 COMPATIBLE_LANGOPT(ModulesStrictDeclUse, 1, 0, "requiring declaration of 
module uses and all headers to be in modules")


Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -981,6 +981,14 @@
  LateParsedInstantiations.begin(),
  LateParsedInstantiations.end());
 LateParsedInstantiations.clear();
+
+// FIXME: Instantiating implicit templates already in the PCH breaks some
+// OpenMP-specific code paths, see https://reviews.llvm.org/D69585 .
+if(LangOpts.PCHInstantiateTemplates && !LangOpts.OpenMP) {
+  llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
+ StringRef(""));
+  PerformPendingInstantiations();
+}
   }
 
   DiagnoseUnterminatedPragmaPack();
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cp

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak updated this revision to Diff 227575.
llunak added a comment.

Let's go a different route. This patch fully passes all tests and so should be 
ready to be committed.

It does so by opting out for OpenMP, which can be handled later whenever 
somebody who know about OpenMP looks at it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69585

Files:
  clang/lib/Sema/Sema.cpp
  clang/test/CodeGenCXX/vla-lambda-capturing.cpp


Index: clang/test/CodeGenCXX/vla-lambda-capturing.cpp
===
--- clang/test/CodeGenCXX/vla-lambda-capturing.cpp
+++ clang/test/CodeGenCXX/vla-lambda-capturing.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 %s -std=c++11 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -std=c++11 -emit-llvm -o - | FileCheck --check-prefixes 
CHECK,CHECK-NOPCH %s
 // RUN: %clang_cc1 %s -std=c++11 -emit-pch -o %t
-// RUN: %clang_cc1 %s -std=c++11 -include-pch %t -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -std=c++11 -include-pch %t -emit-llvm -o - | FileCheck 
--check-prefixes CHECK,CHECK-PCH %s
 
 #ifndef HEADER
 #define HEADER
@@ -111,14 +111,14 @@
 // CHECK: call void @llvm.stackrestore(
 // CHECK: ret void
 
-// CHECK: define linkonce_odr{{.*}} void [[F_INT_LAMBDA]]([[CAP_TYPE2]]*
-// CHECK: [[THIS:%.+]] = load [[CAP_TYPE2]]*, [[CAP_TYPE2]]**
-// CHECK: [[SIZE_REF:%.+]] = getelementptr inbounds [[CAP_TYPE2]], 
[[CAP_TYPE2]]* [[THIS]], i{{.+}} 0, i{{.+}} 0
-// CHECK: [[SIZE:%.+]] = load [[INTPTR_T]], [[INTPTR_T]]* [[SIZE_REF]]
-// CHECK: call i{{.+}}* @llvm.stacksave()
-// CHECK: alloca [[INTPTR_T]], [[INTPTR_T]] [[SIZE]]
-// CHECK: call void @llvm.stackrestore(
-// CHECK: ret void
+// CHECK-NOPCH: define linkonce_odr{{.*}} void [[F_INT_LAMBDA]]([[CAP_TYPE2]]*
+// CHECK-NOPCH: [[THIS:%.+]] = load [[CAP_TYPE2]]*, [[CAP_TYPE2]]**
+// CHECK-NOPCH: [[SIZE_REF:%.+]] = getelementptr inbounds [[CAP_TYPE2]], 
[[CAP_TYPE2]]* [[THIS]], i{{.+}} 0, i{{.+}} 0
+// CHECK-NOPCH: [[SIZE:%.+]] = load [[INTPTR_T]], [[INTPTR_T]]* [[SIZE_REF]]
+// CHECK-NOPCH: call i{{.+}}* @llvm.stacksave()
+// CHECK-NOPCH: alloca [[INTPTR_T]], [[INTPTR_T]] [[SIZE]]
+// CHECK-NOPCH: call void @llvm.stackrestore(
+// CHECK-NOPCH: ret void
 
 // CHECK: define linkonce_odr{{.*}} void [[B_INT_LAMBDA]]([[CAP_TYPE3]]*
 // CHECK: [[SIZE2_REF:%.+]] = getelementptr inbounds [[CAP_TYPE3]], 
[[CAP_TYPE3]]* [[THIS:%.+]], i{{[0-9]+}} 0, i{{[0-9]+}} 1
@@ -168,4 +168,14 @@
 // CHECK: [[MUL:%.+]] = mul {{.*}} i{{[0-9]+}} [[SIZE2]], [[SIZE1]]
 // CHECK: mul {{.*}} i{{[0-9]+}} {{[0-9]+}}, [[MUL]]
 // CHECK: ret void
+
+// CHECK-PCH: define linkonce_odr{{.*}} void [[F_INT_LAMBDA]]([[CAP_TYPE2]]*
+// CHECK-PCH: [[THIS:%.+]] = load [[CAP_TYPE2]]*, [[CAP_TYPE2]]**
+// CHECK-PCH: [[SIZE_REF:%.+]] = getelementptr inbounds [[CAP_TYPE2]], 
[[CAP_TYPE2]]* [[THIS]], i{{.+}} 0, i{{.+}} 0
+// CHECK-PCH: [[SIZE:%.+]] = load [[INTPTR_T]], [[INTPTR_T]]* [[SIZE_REF]]
+// CHECK-PCH: call i{{.+}}* @llvm.stacksave()
+// CHECK-PCH: alloca [[INTPTR_T]], [[INTPTR_T]] [[SIZE]]
+// CHECK-PCH: call void @llvm.stackrestore(
+// CHECK-PCH: ret void
+
 #endif
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -981,6 +981,14 @@
  LateParsedInstantiations.begin(),
  LateParsedInstantiations.end());
 LateParsedInstantiations.clear();
+
+// FIXME: Instantiating implicit templates already in the PCH breaks some
+// OpenMP-specific code paths, see https://reviews.llvm.org/D69585 .
+if (!LangOpts.OpenMP) {
+  llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
+ StringRef(""));
+  PerformPendingInstantiations();
+}
   }
 
   DiagnoseUnterminatedPragmaPack();


Index: clang/test/CodeGenCXX/vla-lambda-capturing.cpp
===
--- clang/test/CodeGenCXX/vla-lambda-capturing.cpp
+++ clang/test/CodeGenCXX/vla-lambda-capturing.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 %s -std=c++11 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -std=c++11 -emit-llvm -o - | FileCheck --check-prefixes CHECK,CHECK-NOPCH %s
 // RUN: %clang_cc1 %s -std=c++11 -emit-pch -o %t
-// RUN: %clang_cc1 %s -std=c++11 -include-pch %t -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -std=c++11 -include-pch %t -emit-llvm -o - | FileCheck --check-prefixes CHECK,CHECK-PCH %s
 
 #ifndef HEADER
 #define HEADER
@@ -111,14 +111,14 @@
 // CHECK: call void @llvm.stackrestore(
 // CHECK: ret void
 
-// CHECK: define linkonce_odr{{.*}} void [[F_INT_LAMBDA]]([[CAP_TYPE2]]*
-// CHECK: [[THIS:%.+]] = load [[CAP_TYPE2]]*, [[CAP_TYPE2]]**
-// CHECK: [[SIZE_REF:%.+]] = getelementptr inbounds [[CAP_TYPE2]], [[CAP_TYPE2]]* [[THIS]], i{{.+}} 0, i{{.+}} 0
-// CHECK: [[SIZE:%.+]] = load [[INTPTR_T]], [[INTP

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

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

llunak created this revision.
llunak added reviewers: ABataev, rsmith, dblaikie.
llunak added projects: clang, OpenMP.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: cfe-commits.

This patch makes template instantiations be already performed in the PCH 
instead of it being done in every single file that uses the PCH. I can see 
20-30% build time saved with the few tests I've tried.

The delaying of template instantiations until the PCH is used comes from 
7f76d11dcc9196e1fc9d1308da9ed2330a6b06c2 , which doesn't really give any useful 
reasoning for it, except for extending a unittest, which however now passes 
even with the instantiation moved back into the PCH. Since modules do not delay 
instantiation either, presumably whatever was wrong there has been fixed 
already.

I've built a complete debug build of LibreOffice with the patch and everything 
seems to work fine.

All Clang tests pass fine as well, with the exception of 23 tests, 22 of which 
are in OpenMP:

- OpenMP/declare_target_codegen.cpp fails because some virtuals of template 
classes are not emitted. This is fixed by the second change in the patch. I do 
not understand the purpose of that code (neither I have any clue about OpenMP), 
but it was introduced in d2c1dd5902782fb773c64dbf4e0b529aa4044b05 , which also 
changed this very test, and the change makes the test pass again.
- The remaining 22 tests, CodeGenCXX/vla-lambda-capturing.cpp and 21 in 
OpenMP/, all use the same FileCheck for normal compilation and using the source 
file as PCH input. Instantiating already in the PCH reorders some things, which 
makes the tests fail. Looking at the differences in the generated output, they 
all seem to be functionally equivalent to me, they just do not match exactly 
anymore.

That obviously makes the patch not ready, but I'd like to get feedback on this, 
before spending the time on adjusting the tests (which I expect will be quite a 
hassle[*]). So, is there any problem with this patch, besides adjusting the 
tests? And is there some easier way to handle those 21 OpenMP tests besides 
possibly (up to) doubling the checks in them?

[X] BTW, https://pastebin.com/Dg2L7K27 helped quite a lot with reviewing the 
differences.


Repository:
  rC Clang

https://reviews.llvm.org/D69585

Files:
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDeclCXX.cpp


Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -15569,7 +15569,7 @@
 return;
   // Do not mark as used if compiling for the device outside of the target
   // region.
-  if (LangOpts.OpenMP && LangOpts.OpenMPIsDevice &&
+  if (TUKind != TU_Prefix && LangOpts.OpenMP && LangOpts.OpenMPIsDevice &&
   !isInOpenMPDeclareTargetContext() &&
   !isInOpenMPTargetExecutionDirective()) {
 if (!DefinitionRequired)
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -981,6 +981,12 @@
  LateParsedInstantiations.begin(),
  LateParsedInstantiations.end());
 LateParsedInstantiations.clear();
+
+{
+  llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
+ StringRef(""));
+  PerformPendingInstantiations();
+}
   }
 
   DiagnoseUnterminatedPragmaPack();


Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -15569,7 +15569,7 @@
 return;
   // Do not mark as used if compiling for the device outside of the target
   // region.
-  if (LangOpts.OpenMP && LangOpts.OpenMPIsDevice &&
+  if (TUKind != TU_Prefix && LangOpts.OpenMP && LangOpts.OpenMPIsDevice &&
   !isInOpenMPDeclareTargetContext() &&
   !isInOpenMPTargetExecutionDirective()) {
 if (!DefinitionRequired)
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -981,6 +981,12 @@
  LateParsedInstantiations.begin(),
  LateParsedInstantiations.end());
 LateParsedInstantiations.clear();
+
+{
+  llvm::TimeTraceScope TimeScope("PerformPendingInstantiations",
+ StringRef(""));
+  PerformPendingInstantiations();
+}
   }
 
   DiagnoseUnterminatedPragmaPack();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits