[PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

[Anastasia]

Summary

The example from the bugzilla triggered two issues:

1. In qualification conversion we are creating an address space conversion for non-pointer and non-reference type.
2. We are not taking an address space from the right type when attempting to cast argument of 'this' to its parameter type.

[Anastasia]

lib/CodeGen/CGClass.cpp

2031

I am a bit unsure if performAddrSpaceCast should be used, but considering that we know that we are not casting a constant it should be fine?

If not any suggestions how to propagate LangAS of 'this' here. Some thoughts I have are:

• Try to list the conversion up in the call stack
• Pass LangAS all the way to here

[rjmccall]

lib/CodeGen/CGClass.cpp

2031

I feel like This should just be in the right address space for the constructor at the point EmitCXXConstructorCall is called. We don't expect this function to do any other semantic conversions. Or is this necessary to handle special-case use of things like trivial default / copy constructors?

lib/Sema/SemaInit.cpp

7324

If this function is being used to do pointee qualification conversions on pointer r-values, I think you need to look at whether the pointee address spaces are different, right?

[brunodf]

lib/CodeGen/CGClass.cpp

2031

Where could the conversion of this be listed in the clang AST? this seems implicit there.

Passing along LangAS seems to have some precedent. EmitCXXConstructExpr (which calls EmitCXXConstructorCall) works on AggValueSlot which carries the original qualifiers. Currently not yet used for address space (but this seems similar to me):

/// \param quals - The qualifiers that dictate how the slot should
/// be initialied. Only 'volatile' and the Objective-C lifetime
/// qualifiers matter.

[rjmccall]

lib/CodeGen/CGClass.cpp

2031

Passing down the address space also works, whether in an AggValueSlot or an LValue or whatever, and I suppose it's better for the special cases for trivial constructors.

[Anastasia]

• Use AggValueSlot in the constructor call generation to store/retrieve address space of 'this'.
• Fixed detecting the address space conversion while performing qualification conversion.

[rjmccall]

LGTM.

[rjmccall]

test/CodeGenCXX/address-space-of-this.cpp

10

Sorry I didn't catch this before, but I don't see why this test is expected to work. We can't actually pass a pointer in address space 10 to that constructor.

[Anastasia]

test/CodeGenCXX/address-space-of-this.cpp

10

Ok, I have created a bug to follow up on this issues:
https://bugs.llvm.org/show_bug.cgi?id=41730

It seems that the check is missing here for allowing the address space conversions implicitly, but I am afraid if I add it now addr spaces will become less usable because implicit conversions can't be setup by the target yet. And everyone is using no address space as some sort of __generic but unofficially. :(

I have some thoughts about adding something like __generic address space to C++ that can either be resolved by the compiler or supported by HW. I think it might help to implement those cases correctly without modifying too much of code base. I just struggled to find enough bandwidth to send an RFC but I will try to progress on this asap.

[rjmccall]

test/CodeGenCXX/address-space-of-this.cpp

10

Ok, I have created a bug to follow up on this issues:

Thanks.

It seems that the check is missing here for allowing the address space conversions implicitly, but I am afraid if I add it now addr spaces will become less usable because implicit conversions can't be setup by the target yet. And everyone is using no address space as some sort of __generic but unofficially. :(

As far as I'm concerned, address-space support in the C++ feature set is all still extremely experimental and there are no real users that we have to worry about making things less useful for. The right thing for the basic model is for constructors to only work when the object is being constructed in an address space that's convertible to the address space of the constructor. Languages with a __generic superspace (whether implemented with dynamic selection or cloning or anything else) can consider making it the default address space of constructors, but that's not something we should be pushing in the basic model.

[Anastasia]

test/CodeGenCXX/address-space-of-this.cpp

10

As far as I'm concerned, address-space support in the C++ feature set is all still extremely experimental and there are no real users that we have to worry about making things less useful for.

Ok, then I can try to fix that generically. And at least we can test it using __constant in OpenCL.

The right thing for the basic model is for constructors to only work when the object is being constructed in an address space that's convertible to the address space of the constructor.

Just to understand a bit more. Would the constructor address space be given in the source code or would it be up to the target to set it up? Also would it be applied to all other methods too?

Languages with a __generic superspace (whether implemented with dynamic selection or cloning or anything else) can consider making it the default address space of constructors, but that's not something we should be pushing in the basic model.

Ok, I have drafted an RFC around this topic that I was planning to share with Clang dev community at some point.
https://docs.google.com/document/d/1YybeeRgGrckMjxjtLdRUf0V5f9Psq97cWxBN4npDoqk/edit?usp=sharing

Just in short the idea of this proposal is to make something like __generic address space as a feature configurable by the targets. It is extending the original idea described in http://lists.llvm.org/pipermail/cfe-dev/2019-March/061541.html

I am also discussing briefly some extra ideas of language based solutions to provide information on address spaces for class instantiations at the source level. Your feedback would be highly appreciated even at this very early stage (of course if you have any extra bandwidth). It would be much easier to fix remaining issues once we agree on the future directions. Thanks!

[rjmccall]

test/CodeGenCXX/address-space-of-this.cpp

10

Just to understand a bit more. Would the constructor address space be given in the source code or would it be up to the target to set it up? Also would it be applied to all other methods too?

1. It should be possible to give a method / constructor an address space explicitly in source code. This would be treated basically like a const or volatile qualifier on the method would in terms of overloading, mangling, etc. To support this properly for constructors will require some refactoring in Sema to propagate the address space of the object we're constructing into the initialization / overload-resolution code — usually that'll be LangAS::Default, but there are other contexts where we can pick up other address spaces, like if we're initializing a global (and the target knows how to allocate in that address space.

1a. As a corollary, we should allow operator new to be address-space qualified, which is weirder (because it's not a non-static member function) but I think the only reasonable way to manage that.

2. I think it's reasonable to have language modes and targets be able to set a default address space for methods and constructors that isn't necessarily LangAS::Default. Note that this implies that there needs to be some way to spell LangAS::Default on the target, though, since it's almost surely important to be able to declare methods there.

At some point I'm not sure that cfe-dev is even the right place for this; you might want to write up some sort of report to the C++ committee about recommendations for extending C++ to generalized address spaces, in the interest of creating a sort of Embedded C++ spec.

[Anastasia]

test/CodeGenCXX/address-space-of-this.cpp

10

It should be possible to give a method / constructor an address space explicitly in source code. This would be treated basically like a const or volatile qualifier on the method would in terms of overloading, mangling, etc. To support this properly for constructors will require some refactoring in Sema to propagate the address space of the object we're constructing into the initialization / overload-resolution code — usually that'll be LangAS::Default, but there are other contexts where we can pick up other address spaces, like if we're initializing a global (and the target knows how to allocate in that address space.

1a. As a corollary, we should allow operator new to be address-space qualified, which is weirder (because it's not a non-static member function) but I think the only reasonable way to manage that.

I think it's reasonable to have language modes and targets be able to set a default address space for methods and constructors that isn't necessarily LangAS::Default. Note that this implies that there needs to be some way to spell LangAS::Default on the target, though, since it's almost surely important to be able to declare methods there.

Ok, that seems to align with what I was thinking to implement. Btw, I have a fix to prevent incorrect addr space cast while constructing objects that covers this test: https://reviews.llvm.org/D62156

At some point I'm not sure that cfe-dev is even the right place for this; you might want to write up some sort of report to the C++ committee about recommendations for extending C++ to generalized address spaces, in the interest of creating a sort of Embedded C++ spec.

I agree this should be the end goal. However, I feel a lot more confident to write the proposal if I have a working tool. Also I would like to leverage the diverse knowledge in Clang community to make sure the ideas cover variety of different architecture and use cases. I believe it might take some time until we get something more concrete in the spec and having some sort of working tool would allow us to provide temporary base for some solutions.

lib/CodeGen/CGClass.cpp

// before the construction of the next array element, if any.
     // Evaluate the constructor and its arguments in a regular
     // partial-destroy cleanup.
     if (getLangOpts().Exceptions &&
         !ctor->getParent()->hasTrivialDestructor()) {
       Destroyer *destroyer = destroyCXXObject;
       pushRegularPartialArrayCleanup(arrayBegin, cur, type, eltAlignment,
                                      *destroyer);
     }
-
+    auto currAVS = AggValueSlot::forAddr(
+        curAddr, type.getQualifiers(), AggValueSlot::IsDestructed,
+        AggValueSlot::DoesNotNeedGCBarriers, AggValueSlot::IsNotAliased,
+        AggValueSlot::DoesNotOverlap, AggValueSlot::IsNotZeroed,
+        NewPointerIsChecked ? AggValueSlot::IsSanitizerChecked
+                            : AggValueSlot::IsNotSanitizerChecked);
     EmitCXXConstructorCall(ctor, Ctor_Complete, /*ForVirtualBase=*/false,
-                           /*Delegating=*/false, curAddr, E,
-                           AggValueSlot::DoesNotOverlap, NewPointerIsChecked);
+                           /*Delegating=*/false, currAVS, E);
   }
 
   // Go to the next element.
   llvm::Value *next =
     Builder.CreateInBoundsGEP(cur, llvm::ConstantInt::get(SizeTy, 1),
                               "arrayctor.next");
   cur->addIncoming(next, Builder.GetInsertBlock());
 
void CodeGenFunction::destroyCXXObject(CodeGenFunction &CGF,
   assert(!dtor->isTrivial());
   CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false,
                             /*Delegating=*/false, addr);
 }
 
 void CodeGenFunction::EmitCXXConstructorCall(const CXXConstructorDecl *D,
                                              CXXCtorType Type,
                                              bool ForVirtualBase,
-                                             bool Delegating, Address This,
-                                             const CXXConstructExpr *E,
-                                             AggValueSlot::Overlap_t Overlap,
-                                             bool NewPointerIsChecked) {
+                                             bool Delegating,
+                                             AggValueSlot ThisAVS,
+                                             const CXXConstructExpr *E) {
   CallArgList Args;
-
-  LangAS SlotAS = E->getType().getAddressSpace();
+  Address This = ThisAVS.getAddress();
+  LangAS SlotAS = ThisAVS.getQualifiers().getAddressSpace();
   QualType ThisType = D->getThisType();
   LangAS ThisAS = ThisType.getTypePtr()->getPointeeType().getAddressSpace();
   llvm::Value *ThisPtr = This.getPointer();
+
   if (SlotAS != ThisAS) {
     unsigned TargetThisAS = getContext().getTargetAddressSpace(ThisAS);
     llvm::Type *NewType =
         ThisPtr->getType()->getPointerElementType()->getPointerTo(TargetThisAS);
     ThisPtr = getTargetHooks().performAddrSpaceCast(*this, This.getPointer(),
                                                     ThisAS, SlotAS, NewType);
   }
+
   // Push the this ptr.
   Args.add(RValue::get(ThisPtr), D->getThisType());
 
   // If this is a trivial constructor, emit a memcpy now before we lose
   // the alignment information on the argument.
   // FIXME: It would be better to preserve alignment information into CallArg.
   if (isMemcpyEquivalentSpecialMember(D)) {
     assert(E->getNumArgs() == 1 && "unexpected argcount for trivial ctor");
 
     const Expr *Arg = E->getArg(0);
     LValue Src = EmitLValue(Arg);
     QualType DestTy = getContext().getTypeDeclType(D->getParent());
     LValue Dest = MakeAddrLValue(This, DestTy);
-    EmitAggregateCopyCtor(Dest, Src, Overlap);
+    EmitAggregateCopyCtor(Dest, Src, ThisAVS.mayOverlap());
     return;
   }
 
   // Add the rest of the user-supplied arguments.
   const FunctionProtoType *FPT = D->getType()->castAs<FunctionProtoType>();
   EvaluationOrder Order = E->isListInitialization()
                               ? EvaluationOrder::ForceLeftToRight
                               : EvaluationOrder::Default;
   EmitCallArgs(Args, FPT, E->arguments(), E->getConstructor(),
                /*ParamsToSkip*/ 0, Order);
 
   EmitCXXConstructorCall(D, Type, ForVirtualBase, Delegating, This, Args,
-                         Overlap, E->getExprLoc(), NewPointerIsChecked);
+                         ThisAVS.mayOverlap(), E->getExprLoc(),
+                         ThisAVS.isSanitizerChecked());
 }
 
 static bool canEmitDelegateCallArgs(CodeGenFunction &CGF,
                                     const CXXConstructorDecl *Ctor,
                                     CXXCtorType Type, CallArgList &Args) {
   // We can't forward a variadic call.
   if (Ctor->isVariadic())
     return false;

lib/CodeGen/CGExprCXX.cpp

case CXXConstructExpr::CK_Complete:
       break;
 
      case CXXConstructExpr::CK_VirtualBase:
       ForVirtualBase = true;
       LLVM_FALLTHROUGH;
 
      case CXXConstructExpr::CK_NonVirtualBase:
       Type = Ctor_Base;
      }
 
      // Call the constructor.
-    EmitCXXConstructorCall(CD, Type, ForVirtualBase, Delegating,
-                           Dest.getAddress(), E, Dest.mayOverlap(),
-                           Dest.isSanitizerChecked());
+     EmitCXXConstructorCall(CD, Type, ForVirtualBase, Delegating, Dest, E);
   }
 }
 
 void CodeGenFunction::EmitSynthesizedCXXCopyCtor(Address Dest, Address Src,
                                                  const Expr *Exp) {
   if (const ExprWithCleanups *E = dyn_cast<ExprWithCleanups>(Exp))
     Exp = E->getSubExpr();
   assert(isa<CXXConstructExpr>(Exp) &&

lib/CodeGen/CodeGenFunction.h

public:
   /// a copy).
   void EmitInheritedCXXConstructorCall(const CXXConstructorDecl *D,
                                        bool ForVirtualBase, Address This,
                                        bool InheritedFromVBase,
                                        const CXXInheritedCtorInitExpr *E);
 
   void EmitCXXConstructorCall(const CXXConstructorDecl *D, CXXCtorType Type,
                               bool ForVirtualBase, bool Delegating,
-                              Address This, const CXXConstructExpr *E,
-                              AggValueSlot::Overlap_t Overlap,
-                              bool NewPointerIsChecked);
+                              AggValueSlot ThisAVS, const CXXConstructExpr *E);
 
   void EmitCXXConstructorCall(const CXXConstructorDecl *D, CXXCtorType Type,
                               bool ForVirtualBase, bool Delegating,
                               Address This, CallArgList &Args,
                               AggValueSlot::Overlap_t Overlap,
-                              SourceLocation Loc,
-                              bool NewPointerIsChecked);
+                              SourceLocation Loc, bool NewPointerIsChecked);
 
   /// Emit assumption load for all bases. Requires to be be called only on
   /// most-derived class and not under construction of the object.
   void EmitVTableAssumptionLoads(const CXXRecordDecl *ClassDecl, Address This);
 
   /// Emit assumption that vptr load == global vtable.
   void EmitVTableAssumptionLoad(const VPtr &vptr, Address This);
 

lib/Sema/SemaInit.cpp

if (RequireCompleteType(E->getExprLoc(), T, diag::err_incomplete_type))
     return ExprError();
 
   return CreateMaterializeTemporaryExpr(E->getType(), E, false);
 }
 
 ExprResult Sema::PerformQualificationConversion(Expr *E, QualType Ty,
                                                 ExprValueKind VK,
                                                 CheckedConversionKind CCK) {
-  CastKind CK = (Ty.getAddressSpace() != E->getType().getAddressSpace())
-                    ? CK_AddressSpaceConversion
-                    : CK_NoOp;
+
+  CastKind CK = CK_NoOp;
+
+  if (VK == VK_RValue) {
+    auto PointeeTy = Ty->getPointeeType();
+    auto ExprPointeeTy = E->getType()->getPointeeType();
+    if (!PointeeTy.isNull() &&
+        PointeeTy.getAddressSpace() != ExprPointeeTy.getAddressSpace())
+      CK = CK_AddressSpaceConversion;
+  } else if (Ty.getAddressSpace() != E->getType().getAddressSpace()) {
+    CK = CK_AddressSpaceConversion;
+  }
+
   return ImpCastExprToType(E, Ty, CK, VK, /*BasePath=*/nullptr, CCK);
 }
 
 ExprResult InitializationSequence::Perform(Sema &S,
                                            const InitializedEntity &Entity,
                                            const InitializationKind &Kind,
                                            MultiExprArg Args,
                                            QualType *ResultType) {

test/CodeGenCXX/address-space-of-this.cpp

+// RUN: %clang_cc1 %s -std=c++14 -triple=spir -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -std=c++17 -triple=spir -emit-llvm -o - | FileCheck %s
+
+struct MyType {
+  MyType(int i) : i(i) {}
+  int i;
+};
+//CHECK: call void @_ZN6MyTypeC1Ei(%struct.MyType* addrspacecast (%struct.MyType addrspace(10)* @m to %struct.MyType*), i32 123)
+MyType __attribute__((address_space(10))) m = 123;