Sunil_Srivastava updated this revision to Diff 71432.
Sunil_Srivastava added a comment.
This is an update to address points raised by Richard Smith:
1. The bit and the access functions have been renamed from
MarkedForPendingInstantiation to InstantiationIsPending.
2. It closely, though not entirely, tracks whether the function is on the
PendingInstantiations list. More on this point below.
3. The test explicitly allows devirtualization if Function->isDefined(). This
is also needed by the change in point 2 above.
4. The test has been enhanced to have PCH tests.
Now: Why the InstantiationIsPending bit is not precisely tracking the presence
in the PendingInstantiations list?
Basically this is because I think that the call to Func2 in the test SHOULD get
devirtualized. Func2 is not defined in this TU, an uncommon but possible
situation. Given that, the compiler had no way to instantiate it, and it is a
user error if Func2 does not get instantiated somewhere else. If Func2 does get
instantiated somewhere else, then it is safe to devirtualize calls to it.
In contrast, operator[] is defined by the user, but for some reason (which will
be removed by my next checkin, in situations we know of) the compiler has
decided to not instantiate it (or rather, not decided to instantiate it, to be
precise). In this case we do not want to devirtualize call to it, because the
definition is not required to exist somewhere else. The whole motivation of
this commit is to prevent such calls from devirtualization.
The instantiation loop removes items from the PendingInstantiations list and
instantiates them, if the body is present. In the case of Func2, the body is
not present, the function has already been removed from the list, yet it is not
isDefined(). We need some way to distinguish this from the contrasting case of
operator[], where the function was never put on the PendingInstantiations list.
Hence in cases like Func2, I am not unsetting the InstantiationIsPending bit at
the time of its removal from the list. Loosely speaking, we can say that the
instantiation is indeed pending, though in some other TU.
Comments in Decls.h explain this behavior, though not in such details.
The expected behavior of the test will change by my next “first fix” commit, of
course; in that the operator[] will get instantiated, and the call will be
devirtualized.
https://reviews.llvm.org/D22057
Files:
include/clang/AST/Decl.h
lib/CodeGen/CGClass.cpp
lib/Sema/Sema.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGen/no-devirt.cpp
Index: test/CodeGen/no-devirt.cpp
===================================================================
--- test/CodeGen/no-devirt.cpp
+++ test/CodeGen/no-devirt.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 %s -DUSEIT -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %s
+
+// Test with decls and template defs in pch, and just use in .cpp
+// RUN: %clang_cc1 %s -DTMPL_DEF_IN_HEADER -triple %itanium_abi_triple -emit-pch -o %t
+// RUN: %clang_cc1 %s -DTMPL_DEF_IN_HEADER -DUSEIT -triple %itanium_abi_triple -include-pch %t -emit-llvm -o - | FileCheck %s
+
+// Test with A in pch, and B and C in main
+// Test with just decls in pch, and template defs and use in .cpp
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -emit-pch -o %t
+// RUN: %clang_cc1 %s -DUSEIT -triple %itanium_abi_triple -include-pch %t -emit-llvm -o - | FileCheck %s
+
+#ifndef HEADER
+#define HEADER
+template < typename T, int N = 0 > class TmplWithArray {
+public:
+ virtual T& operator [] (int idx);
+ virtual T& func1 (int idx);
+ virtual T& func2 (int idx);
+ T ar[N+1];
+};
+struct Wrapper {
+ TmplWithArray<bool, 10> data;
+ bool indexIt(int a) {
+ if (a > 6) return data[a] ; // Should not devirtualize
+ if (a > 4) return data.func1(a); // Should devirtualize
+ return data.func2(a); // Should devirtualize
+ }
+};
+
+#ifdef TMPL_DEF_IN_HEADER
+template <typename T, int N> T& TmplWithArray<T, N >::operator[](int idx) {
+ return ar[idx];
+}
+template <typename T, int N> T& TmplWithArray<T, N >::func1(int idx) {
+ return ar[idx];
+}
+#endif // TMPL_DEF_IN_HEADER
+#endif // HEADER
+
+#ifdef USEIT
+#ifndef TMPL_DEF_IN_HEADER
+template <typename T, int N> T& TmplWithArray<T, N >::operator[](int idx) {
+ return ar[idx];
+}
+template <typename T, int N> T& TmplWithArray<T, N >::func1(int idx) {
+ return ar[idx];
+}
+#endif // !TMPL_DEF_IN_HEADER
+extern Wrapper ew;
+bool stuff(int p)
+{
+ return ew.indexIt(p);
+}
+#endif
+
+// CHECK-NOT: call {{.*}} @_ZN13TmplWithArrayIbLi10EEixEi
+// CHECK-DAG: call {{.*}} @_ZN13TmplWithArrayIbLi10EE5func1Ei
+// CHECK-DAG: call {{.*}} @_ZN13TmplWithArrayIbLi10EE5func2Ei
+
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===================================================================
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3580,6 +3580,7 @@
// Try again at the end of the translation unit (at which point a
// definition will be required).
assert(!Recursive);
+ Function->setInstantiationIsPending(true);
PendingInstantiations.push_back(
std::make_pair(Function, PointOfInstantiation));
} else if (TSK == TSK_ImplicitInstantiation) {
@@ -3599,6 +3600,7 @@
// Postpone late parsed template instantiations.
if (PatternDecl->isLateTemplateParsed() &&
!LateTemplateParser) {
+ Function->setInstantiationIsPending(true);
PendingInstantiations.push_back(
std::make_pair(Function, PointOfInstantiation));
return;
@@ -4925,6 +4927,8 @@
TSK_ExplicitInstantiationDefinition;
InstantiateFunctionDefinition(/*FIXME:*/Inst.second, Function, true,
DefinitionRequired, true);
+ if (Function->isDefined())
+ Function->setInstantiationIsPending(false);
continue;
}
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -13216,6 +13216,7 @@
// call to such a function.
InstantiateFunctionDefinition(PointOfInstantiation, Func);
else {
+ Func->setInstantiationIsPending(true);
PendingInstantiations.push_back(std::make_pair(Func,
PointOfInstantiation));
// Notify the consumer that a function was implicitly instantiated.
Index: lib/Sema/Sema.cpp
===================================================================
--- lib/Sema/Sema.cpp
+++ lib/Sema/Sema.cpp
@@ -679,6 +679,9 @@
// Load pending instantiations from the external source.
SmallVector<PendingImplicitInstantiation, 4> Pending;
ExternalSource->ReadPendingInstantiations(Pending);
+ for (auto PII : Pending)
+ if (auto Func = dyn_cast<FunctionDecl>(PII.first))
+ Func->setInstantiationIsPending(true);
PendingInstantiations.insert(PendingInstantiations.begin(),
Pending.begin(), Pending.end());
}
Index: lib/CodeGen/CGClass.cpp
===================================================================
--- lib/CodeGen/CGClass.cpp
+++ lib/CodeGen/CGClass.cpp
@@ -2909,10 +2909,19 @@
// We can devirtualize calls on an object accessed by a class member access
// expression, since by C++11 [basic.life]p6 we know that it can't refer to
- // a derived class object constructed in the same location.
+ // a derived class object constructed in the same location. However, we avoid
+ // devirtualizing a call to a template function that we could instantiate
+ // implicitly, but have not decided to do so. This is needed because if this
+ // function does not get instantiated, the devirtualization will create a
+ // direct call to a function whose body may not exist. In contrast, calls to
+ // template functions that are not defined in this TU are allowed to be
+ // devirtualized under assumption that it is user responsibility to
+ // instantiate them in some other TU.
if (const MemberExpr *ME = dyn_cast<MemberExpr>(Base))
if (const ValueDecl *VD = dyn_cast<ValueDecl>(ME->getMemberDecl()))
- return VD->getType()->isRecordType();
+ return VD->getType()->isRecordType() &&
+ (MD->instantiationIsPending() || MD->isDefined() ||
+ !MD->isImplicitlyInstantiable());
// We can always devirtualize calls on temporary object expressions.
if (isa<CXXConstructExpr>(Base))
Index: include/clang/AST/Decl.h
===================================================================
--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -1599,6 +1599,7 @@
unsigned HasImplicitReturnZero : 1;
unsigned IsLateTemplateParsed : 1;
unsigned IsConstexpr : 1;
+ unsigned InstantiationIsPending:1;
/// \brief Indicates if the function uses __try.
unsigned UsesSEHTry : 1;
@@ -1691,7 +1692,8 @@
HasWrittenPrototype(true), IsDeleted(false), IsTrivial(false),
IsDefaulted(false), IsExplicitlyDefaulted(false),
HasImplicitReturnZero(false), IsLateTemplateParsed(false),
- IsConstexpr(isConstexprSpecified), UsesSEHTry(false),
+ IsConstexpr(isConstexprSpecified),
+ InstantiationIsPending(false), UsesSEHTry(false),
HasSkippedBody(false), EndRangeLoc(NameInfo.getEndLoc()),
TemplateOrSpecialization(),
DNLoc(NameInfo.getInfo()) {}
@@ -1883,6 +1885,15 @@
bool isConstexpr() const { return IsConstexpr; }
void setConstexpr(bool IC) { IsConstexpr = IC; }
+ /// \brief Whether the instantiation of this function is pending.
+ /// This bit is set when the decision to instantiate this function is made
+ /// and unset if and when the function body is created. That leaves out
+ /// cases where instantiation did not happen because the template definition
+ /// was not seen in this TU. This bit remains set in those cases, under the
+ /// assumption that the instantiation will happen in some other TU.
+ bool instantiationIsPending() const { return InstantiationIsPending; }
+ void setInstantiationIsPending(bool IC) { InstantiationIsPending = IC; }
+
/// \brief Indicates the function uses __try.
bool usesSEHTry() const { return UsesSEHTry; }
void setUsesSEHTry(bool UST) { UsesSEHTry = UST; }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
