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

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

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

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 /

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

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

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

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

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

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?

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

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

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

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/

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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?

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

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 .

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

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

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

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

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

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:

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

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

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

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

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

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.

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

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