[C++4OpenCL] Add support for multiple address spaced destructors

[olestrohm]

Summary

This patch aims to add initial support for multiple address spaced destructors.
Currently this parses fine and the destructors are correctly added to the class,
but there is no logic to choose the correct destructor for a variable in a specific address space.
Currently it always picks the first destructor defined in the source code, regardless of the
address space of the variable.

This is not aiming to be a complete implementation, as destructors are used in a very large
number of locations, and so doing all of it in one patch would be very difficult to review and code.

The goals for this patch is thus to add basic support for destructors, and to build a framework for
additional work to be done on this front.
Since this feature won't be completed in this patch, I have attempted to ensure that no C++
functionality is changed, to ensure that the changes will only affect C++ for OpenCL.
This also has the effect that whenever destructors aren't correctly implemented it will fallback
to default behaviour, which is exactly the same currently happens.

In summary destructors in C++ for OpenCL are currently unusable for multiple destructors. With
this patch the feature will be usable in the most common situation, and every bug that currently
exists that isn't covered by the changes I have made here will continue to be bugs. The hope is that
this patch therefore is almost entirely a positive, since while it doesn't fix every bug, it will make the
feature actually work, and will create a base to fix the other bugs as they are discovered.

This is why it's mostly implemented through default parameters, as that will allow this change to
remain as small as it is, while also allowing further fixes to come along later.

The feature is also C++ for OpenCL specific to let the fast path remain when not utilizing the
address spaces, as this new implementation will be slower than the bitfields that C++ currently
uses, but I hope this can also be optimized in the future if it turns out to be very slow.

[Anastasia]

The feature is also C++ for OpenCL specific to let the fast path remain when not utilizing the
address spaces, as this new implementation will be slower than the bitfields that C++ currently
uses, but I hope this can also be optimized in the future if it turns out to be very slow.

In general we try to align OpenCL's address spaces with C/C++ general direction but we haven't completed support of address space methods qualifiers in C++ yet. Therefore supporting dtors with address spaces won't be possible in generic C++. However, it would be good to check that we don't entirely misalign with the C++ requirements.

I would like to see if @rjmccall has any suggestions at this point.

clang/include/clang/AST/DeclCXX.h

983

According to the current OpenCL strategy, we only declare implicit members in default address space (i.e. __generic). It might be that we can declare them for all concrete address spaces but we are not there yet to start adding this feature. So it would make sense, for now, to simplify this logic and only add implicit dtors in the generic address space.

This implies that objects in concrete address spaces would still be able to use such implicit dtors but the compiler would add an implicit conversion to __generic for the implicit object parameter.

clang/include/clang/Sema/Sema.h

4048

it feels to me that this should be named ASThis following the convention?

clang/lib/AST/DeclCXX.cpp

1902

For ctors and other special members the logic for the selection of the overload candidates seems to be implemented at the end of Sema::LookupSpecialMember:
https://clang.llvm.org/doxygen/SemaLookup_8cpp_source.html#l03114

There it creates an implicit parameter this and performs regular overload resolution.

Currently dtors don't hit that point because the implementation fast paths them in that function.

Do you think it makes sense to follow the same logic for dtors (by preventing the early exit there), even though perhaps we would then only do it for OpenCL to avoid performance regressions for pure C++ implementation? However, I am not entirely convinced when and how that code is called for the special members since it doesn't have address space handling anywhere but clang's behavior seems correct for ctors and assignments at least. Although it is possible that it is only used for some corner cases we haven't encountered yet and hence we have a general bug for special members with address spaces in some situations.

Alternatively, we could just add similar logic here if it makes implementation more efficient. After all the special member implementation doesn't seem to reuse fully common member function overload resolution code so perhaps it is not unreasonable to add a special case for dtors. I am not saying though we need to do it in this path... but it would be good to understand how the evolution of your approach would look like towards completion.

clang/lib/Sema/SemaLookup.cpp

3082

Btw ctors and assignments don't seem to need this but they seem to work fine though...

For example here the right assignment overload with __local address space is selected
https://godbolt.org/z/aYKj4W6rc
or ctor overload with __global is selected here correctly:
https://godbolt.org/z/1frheezE5

So it seems like there is some other logic to handle address spaces for special members elsewhere? Although it is very strange and rather confusing.

3102

Currently, all other special members added implicitly use default address space (i.e. __generic for OpenCL) for this via Sema::setupImplicitSpecialMemberType helper so I think we should mirror it for DeclareImplicitDestructor too... then perhaps we can avoid changes in Sema::setupImplicitSpecialMemberType?

FYI in the past we have discussed that we might want to change the design and put special members in concrete address spaces but we haven't gone that far yet and it would make sense to have all special members working coherently... so it would be better for dtors to just mirror logic from the other special members for now.

[keryell]

While it might be possible to extend arbitrarily C++, I have the feeling that having just 1 destructor and having a different code path-code according to the address space could be enough.
It could be possible to write:

~MyDestructor() {
  if constexpr (SomeAPIReturningAddressSpace() == ...::Global) {
    /* Global address space code */
  }
  else if constexpr (...) {
  }
}

It is possible to build higher-level dispatching constructs from this in OpenCL C++ library, for example à la std::visit from std::variant.

[olestrohm]

I made the implicit destructor always be created in __generic address space.

I couldn't manage to properly figure a case that would trigger LookupSpecialMember,
so I couldn't figure out how or if address spaces are handled there.

re: @keryell
This might work, though I have no idea how that SomeAPIReturningAddrSpace would work, since at this point the variable would likely be cast to be in __generic addrspace.
Also I'm not sure how that would interact with deleted destructors.

[olestrohm]

clang/lib/Sema/SemaLookup.cpp

3082

Yes, it seems other special members are handled somewhere else according to my preliminary investigations, so that might explain why address spaces where never introduced into this function.

[keryell]

In D109609#3006225, @olestrohm wrote:

re: @keryell
This might work, though I have no idea how that SomeAPIReturningAddrSpace would work, since at this point the variable would likely be cast to be in __generic addrspace.
Also I'm not sure how that would interact with deleted destructors.

You mean for example say that the constructor in global address-space would have some definition but the constructor in constant is deleted?
Ouch. You are killing my suggestion. :-)

clang/include/clang/AST/DeclCXX.h

 /// Defines the C++ Decl subclasses, other than those for templates
 /// (found in DeclTemplate.h) and friends (in DeclFriend.h).
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_AST_DECLCXX_H
 #define LLVM_CLANG_AST_DECLCXX_H
 
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTUnresolvedSet.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExternalASTSource.h"
 #include "clang/AST/LambdaCapture.h"
 #include "clang/AST/NestedNameSpecifier.h"
public:
   ///
   /// When false, a destructor will be implicitly declared.
   bool hasUserDeclaredDestructor() const {
     return data().UserDeclaredSpecialMembers & SMF_Destructor;
   }
 
   /// Determine whether this class needs an implicit destructor to
   /// be lazily declared.
-  bool needsImplicitDestructor() const {
+  bool needsImplicitDestructor(LangAS AS = LangAS::Default) const {
+    if (getASTContext().getLangOpts().OpenCL) {
+      return getDestructor(AS) == nullptr;
+    }
+
     return !(data().DeclaredSpecialMembers & SMF_Destructor);
   }
 
   /// Determine whether we need to eagerly declare a destructor for this
   /// class.
   bool needsOverloadResolutionForDestructor() const {
     return data().NeedOverloadResolutionForDestructor;
   }
public:
   const CXXRecordDecl *getTemplateInstantiationPattern() const;
 
   CXXRecordDecl *getTemplateInstantiationPattern() {
     return const_cast<CXXRecordDecl *>(const_cast<const CXXRecordDecl *>(this)
                                            ->getTemplateInstantiationPattern());
   }
 
   /// Returns the destructor decl for this class.
-  CXXDestructorDecl *getDestructor() const;
+  CXXDestructorDecl *getDestructor(LangAS AS = LangAS::Default) const;
 
   /// Returns true if the class destructor, or any implicitly invoked
   /// destructors are marked noreturn.
   bool isAnyDestructorNoReturn() const { return data().IsAnyDestructorNoReturn; }
 
   /// If the class is a local class [class.local], returns
   /// the enclosing function declaration.
   const FunctionDecl *isLocalClass() const {

clang/include/clang/Sema/Sema.h

enum LiteralOperatorLookupResult {
     /// passed as a non-type template argument pack.
     LOLR_Template,
     /// The lookup found an overload set of literal operator templates,
     /// which expect the character type and characters of the spelling of the
     /// string literal token to be passed as template arguments.
     LOLR_StringTemplatePack,
   };
 
-  SpecialMemberOverloadResult LookupSpecialMember(CXXRecordDecl *D,
-                                                  CXXSpecialMember SM,
-                                                  bool ConstArg,
-                                                  bool VolatileArg,
-                                                  bool RValueThis,
-                                                  bool ConstThis,
-                                                  bool VolatileThis);
+  SpecialMemberOverloadResult
+  LookupSpecialMember(CXXRecordDecl *D, CXXSpecialMember SM, bool ConstArg,
+                      bool VolatileArg, bool RValueThis, bool ConstThis,
+                      bool VolatileThis, LangAS ASThis = LangAS::Default);
 
   typedef std::function<void(const TypoCorrection &)> TypoDiagnosticGenerator;
   typedef std::function<ExprResult(Sema &, TypoExpr *, TypoCorrection)>
       TypoRecoveryCallback;
 
 private:
   bool CppLookupName(LookupResult &R, Scope *S);
 
public:
   CXXConstructorDecl *LookupCopyingConstructor(CXXRecordDecl *Class,
                                                unsigned Quals);
   CXXMethodDecl *LookupCopyingAssignment(CXXRecordDecl *Class, unsigned Quals,
                                          bool RValueThis, unsigned ThisQuals);
   CXXConstructorDecl *LookupMovingConstructor(CXXRecordDecl *Class,
                                               unsigned Quals);
   CXXMethodDecl *LookupMovingAssignment(CXXRecordDecl *Class, unsigned Quals,
                                         bool RValueThis, unsigned ThisQuals);
-  CXXDestructorDecl *LookupDestructor(CXXRecordDecl *Class);
+  CXXDestructorDecl *LookupDestructor(CXXRecordDecl *Class,
+                                      LangAS AS = LangAS::Default);
 
   bool checkLiteralOperatorId(const CXXScopeSpec &SS, const UnqualifiedId &Id,
                               bool IsUDSuffix);
   LiteralOperatorLookupResult
   LookupLiteralOperator(Scope *S, LookupResult &R, ArrayRef<QualType> ArgTys,
                         bool AllowRaw, bool AllowTemplate,
                         bool AllowStringTemplate, bool DiagnoseMissing,
                         StringLiteral *StringLit = nullptr);

clang/lib/AST/DeclCXX.cpp

if (MemberSpecializationInfo *MSInfo = getMemberSpecializationInfo()) {
     }
   }
 
   assert(!isTemplateInstantiation(this->getTemplateSpecializationKind()) &&
          "couldn't find pattern for class template instantiation");
   return nullptr;
 }
 
-CXXDestructorDecl *CXXRecordDecl::getDestructor() const {
+CXXDestructorDecl *CXXRecordDecl::getDestructor(LangAS AS) const {
   ASTContext &Context = getASTContext();
   QualType ClassType = Context.getTypeDeclType(this);
 
   DeclarationName Name
     = Context.DeclarationNames.getCXXDestructorName(
                                           Context.getCanonicalType(ClassType));
 
   DeclContext::lookup_result R = lookup(Name);
 
+  if (AS == LangAS::Default)
     return R.empty() ? nullptr : dyn_cast<CXXDestructorDecl>(R.front());
+
+  CXXDestructorDecl *Best = nullptr;
+
+  for (DeclContext::lookup_result::iterator I = R.begin(), E = R.end(); I != E;
+       ++I) {
+    auto *Dtor = dyn_cast<CXXDestructorDecl>(*I);
+    LangAS DtorAS = Dtor->getMethodQualifiers().getAddressSpace();
+    if (Qualifiers::isAddressSpaceSupersetOf(DtorAS, AS) &&
+        (Best == nullptr ||
+         Qualifiers::isAddressSpaceSupersetOf(
+             Best->getMethodQualifiers().getAddressSpace(), DtorAS))) {
+      Best = Dtor;
+    }
+  }
+
+  return Best;
 }
 
 static bool isDeclContextInNamespace(const DeclContext *DC) {
   while (!DC->isTranslationUnit()) {
     if (DC->isNamespace())
       return true;
     DC = DC->getParent();
   }

clang/lib/Sema/SemaDeclCXX.cpp

void Sema::DefineInheritingConstructor(SourceLocation CurrentLocation,
   DiagnoseUninitializedFields(*this, Constructor);
 }
 
 CXXDestructorDecl *Sema::DeclareImplicitDestructor(CXXRecordDecl *ClassDecl) {
   // C++ [class.dtor]p2:
   //   If a class has no user-declared destructor, a destructor is
   //   declared implicitly. An implicitly-declared destructor is an
   //   inline public member of its class.
-  assert(ClassDecl->needsImplicitDestructor());
+  assert(ClassDecl->needsImplicitDestructor(getDefaultCXXMethodAddrSpace()));
 
   DeclaringSpecialMember DSM(*this, ClassDecl, CXXDestructor);
   if (DSM.isAlreadyBeingDeclared())
     return nullptr;
 
   bool Constexpr = defaultedSpecialMemberIsConstexpr(*this, ClassDecl,
                                                      CXXDestructor,
                                                      false);
void Sema::FinalizeVarWithDestructor(VarDecl *VD, const RecordType *Record) {
   CXXRecordDecl *ClassDecl = cast<CXXRecordDecl>(Record->getDecl());
   if (ClassDecl->isInvalidDecl()) return;
   if (ClassDecl->hasIrrelevantDestructor()) return;
   if (ClassDecl->isDependentContext()) return;
 
   if (VD->isNoDestroy(getASTContext()))
     return;
 
-  CXXDestructorDecl *Destructor = LookupDestructor(ClassDecl);
+  LangAS AS = LangAS::Default;
+  if (Context.getLangOpts().OpenCL) {
+    AS = VD->getType().getAddressSpace();
+  }
+  CXXDestructorDecl *Destructor = LookupDestructor(ClassDecl, AS);
 
   // If this is an array, we'll require the destructor during initialization, so
   // we can skip over this. We still want to emit exit-time destructor warnings
   // though.
   if (!VD->getType()->isArrayType()) {
     MarkFunctionReferenced(VD->getLocation(), Destructor);
     CheckDestructorAccess(VD->getLocation(), Destructor,
                           PDiag(diag::err_access_dtor_var)

clang/lib/Sema/SemaLookup.cpp

void Sema::LookupOverloadedOperatorName(OverloadedOperatorKind Op, Scope *S,
   DeclarationName OpName = Context.DeclarationNames.getCXXOperatorName(Op);
   LookupResult Operators(*this, OpName, SourceLocation(), LookupOperatorName);
   LookupName(Operators, S);
 
   assert(!Operators.isAmbiguous() && "Operator lookup cannot be ambiguous");
   Functions.append(Operators.begin(), Operators.end());
 }
 
-Sema::SpecialMemberOverloadResult Sema::LookupSpecialMember(CXXRecordDecl *RD,
-                                                           CXXSpecialMember SM,
-                                                           bool ConstArg,
-                                                           bool VolatileArg,
-                                                           bool RValueThis,
-                                                           bool ConstThis,
-                                                           bool VolatileThis) {
+Sema::SpecialMemberOverloadResult
+Sema::LookupSpecialMember(CXXRecordDecl *RD, CXXSpecialMember SM, bool ConstArg,
+                          bool VolatileArg, bool RValueThis, bool ConstThis,
+                          bool VolatileThis, LangAS ASThis) {
   assert(CanDeclareSpecialMemberFunction(RD) &&
          "doing special member lookup into record that isn't fully complete");
   RD = RD->getDefinition();
   if (RValueThis || ConstThis || VolatileThis)
     assert((SM == CXXCopyAssignment || SM == CXXMoveAssignment) &&
            "constructors and destructors always have unqualified lvalue this");
   if (ConstArg || VolatileArg)
     assert((SM != CXXDefaultConstructor && SM != CXXDestructor) &&
            "parameter-less special members can't have qualified arguments");
 
   // FIXME: Get the caller to pass in a location for the lookup.
   SourceLocation LookupLoc = RD->getLocation();
 
   llvm::FoldingSetNodeID ID;
   ID.AddPointer(RD);
   ID.AddInteger(SM);
   ID.AddInteger(ConstArg);
   ID.AddInteger(VolatileArg);
   ID.AddInteger(RValueThis);
   ID.AddInteger(ConstThis);
   ID.AddInteger(VolatileThis);
+  ID.AddInteger((unsigned)ASThis);
 
   void *InsertPoint;
   SpecialMemberOverloadResultEntry *Result =
     SpecialMemberCache.FindNodeOrInsertPos(ID, InsertPoint);
 
   // This was already cached
   if (Result)
     return *Result;
 
   Result = BumpAlloc.Allocate<SpecialMemberOverloadResultEntry>();
   Result = new (Result) SpecialMemberOverloadResultEntry(ID);
   SpecialMemberCache.InsertNode(Result, InsertPoint);
 
   if (SM == CXXDestructor) {
-    if (RD->needsImplicitDestructor()) {
+    if (RD->needsImplicitDestructor(ASThis)) {
       runWithSufficientStackSpace(RD->getLocation(), [&] {
         DeclareImplicitDestructor(RD);
       });
     }
-    CXXDestructorDecl *DD = RD->getDestructor();
+    CXXDestructorDecl *DD = RD->getDestructor(ASThis);
     Result->setMethod(DD);
     Result->setKind(DD && !DD->isDeleted()
                         ? SpecialMemberOverloadResult::Success
                         : SpecialMemberOverloadResult::NoMemberOrDeleted);
     return *Result;
   }
 
   // Prepare for overload resolution. Here we construct a synthetic argument

 }
 
 /// Look for the destructor of the given class.
 ///
 /// During semantic analysis, this routine should be used in lieu of
 /// CXXRecordDecl::getDestructor().
 ///
 /// \returns The destructor for this class.
-CXXDestructorDecl *Sema::LookupDestructor(CXXRecordDecl *Class) {
+CXXDestructorDecl *Sema::LookupDestructor(CXXRecordDecl *Class, LangAS AS) {
   return cast<CXXDestructorDecl>(LookupSpecialMember(Class, CXXDestructor,
-                                                     false, false, false,
-                                                     false, false).getMethod());
+                                                     false, false, false, false,
+                                                     false, AS)
+                                     .getMethod());
 }
 
 /// LookupLiteralOperator - Determine which literal operator should be used for
 /// a user-defined literal, per C++11 [lex.ext].
 ///
 /// Normal overload resolution is not used to select which literal operator to
 /// call for a user-defined literal. Look up the provided literal operator name,
 /// and filter the results to the appropriate set for the given argument types.

clang/test/SemaOpenCLCXX/addrspace-destructors.clcpp

+// RUN: %clang_cc1 %s -pedantic -ast-dump | FileCheck %s
+
+// CHECK: CXXDestructorDecl {{.*}} used ~ExactDtor 'void () __private noexcept'
+struct ExactDtor {
+     ~ExactDtor() __private;
+};
+
+// CHECK: CXXDestructorDecl
+// CHECK-NOT: used
+// CHECK-SAME: ~OverloadedDtor 'void () __generic'
+// CHECK: CXXDestructorDecl {{.*}} used ~OverloadedDtor 'void () __private noexcept'
+struct OverloadedDtor {
+     ~OverloadedDtor() __generic;
+     ~OverloadedDtor() __private;
+};
+
+// CHECK: CXXDestructorDecl
+// CHECK-NOT: used
+// CHECK-SAME: ~ImplicitDtor 'void () __global'
+// CHECK: CXXDestructorDecl {{.*}} implicit used ~ImplicitDtor 'void () __generic noexcept'
+struct ImplicitDtor {
+    ~ImplicitDtor() __global;
+};
+
+// CHECK: CXXDestructorDecl {{.*}} used ~Templated 'void () __generic noexcept'
+// CHECK: CXXDestructorDecl
+// CHECK-NOT: used
+// CHECK-SAME: ~Templated 'void () __global'
+template <typename T>
+struct Templated {
+    ~Templated() __generic;
+    ~Templated() __global;
+};
+
+// CHECK: CXXDestructorDecl {{.*}} used ~BothUsed 'void () __private noexcept'
+// CHECK: CXXDestructorDecl {{.*}} used ~BothUsed 'void () __global noexcept'
+struct BothUsed {
+    ~BothUsed() __private;
+    ~BothUsed() __global;
+};
+
+__global BothUsed g_inheriting;
+
+kernel void k() {
+    __private ExactDtor exact;
+    __private OverloadedDtor overloaded;
+    __private ImplicitDtor implicit;
+    __private Templated<int> templated;
+    __private BothUsed inheriting;
+}