This is really a reference (microsoft#5473)

This fixes a few cases that were missed when I updated `this` objects to
be references. Specifically we were still generating some implicit
`this` cases as pointers, and we were not always correctly looking
through the reference types.

Fixes microsoft#4709

Author: llvm-beanz

tools/clang/include/clang/AST/DeclCXX.h

@@ -1830,6 +1830,13 @@ class CXXMethodDecl : public FunctionDecl {
   /// Should only be called for instance (i.e., non-static) methods.
   QualType getThisType(ASTContext &C) const;
 
+  // HLSL Change Begin - This is a reference.
+  /// \brief Returns the type of the \c this object looking through the pointer.
+  ///
+  /// Should only be called for instance (i.e., non-static) methods.
+  QualType getThisObjectType(ASTContext &C) const;
+  // HLSL Change End - This is a reference.
+
   unsigned getTypeQualifiers() const {
     return getType()->getAs<FunctionProtoType>()->getTypeQuals();
   }

tools/clang/lib/AST/DeclCXX.cpp

@@ -1603,9 +1603,18 @@ QualType CXXMethodDecl::getThisType(ASTContext &C) const {
   QualType ClassTy = C.getTypeDeclType(getParent());
   ClassTy = C.getQualifiedType(ClassTy,
                                Qualifiers::fromCVRMask(getTypeQualifiers()));
-  return C.getPointerType(ClassTy);
+  return C.getLangOpts().HLSL ? C.getLValueReferenceType(ClassTy) : C.getPointerType(ClassTy);
 }
 
+// HLSL Change Begin - This is a reference.
+QualType CXXMethodDecl::getThisObjectType(ASTContext &C) const {
+  QualType ClassTy = C.getTypeDeclType(getParent());
+  ClassTy = C.getQualifiedType(ClassTy,
+                               Qualifiers::fromCVRMask(getTypeQualifiers()));
+  return ClassTy;
+}
+// HLSL Change End - This is a reference.
+
 bool CXXMethodDecl::hasInlineBody() const {
   // If this function is a template instantiation, look at the template from 
   // which it was instantiated.

tools/clang/lib/CodeGen/CodeGenModule.cpp

@@ -1557,8 +1557,10 @@ void CodeGenModule::CompleteDIClassType(const CXXMethodDecl* D) {
 
   if (CGDebugInfo *DI = getModuleDebugInfo())
     if (getCodeGenOpts().getDebugInfo() >= CodeGenOptions::LimitedDebugInfo) {
-      const auto *ThisPtr = cast<PointerType>(D->getThisType(getContext()));
-      DI->getOrCreateRecordType(ThisPtr->getPointeeType(), D->getLocation());
+      // HLSL Change Begin - This is a reference.
+      QualType ThisType = D->getThisObjectType(getContext());
+      DI->getOrCreateRecordType(ThisType, D->getLocation());
+      // HLSL Change End - This is a reference.
     }
 }
 

tools/clang/lib/Sema/SemaExpr.cpp

@@ -1910,13 +1910,15 @@ Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
 
           CXXScopeSpec SS;
           SS.Adopt(ULE->getQualifierLoc());
+          // HLSL Change Begin - This is a reference.
           CXXDependentScopeMemberExpr *DepExpr =
               CXXDependentScopeMemberExpr::Create(
-                  Context, DepThis, DepThisType, true, SourceLocation(),
-                  SS.getWithLocInContext(Context),
-                  ULE->getTemplateKeywordLoc(), nullptr,
-                  R.getLookupNameInfo(),
+                  Context, DepThis, DepThisType,
+                  /*IsArrow*/ !getLangOpts().HLSL, SourceLocation(),
+                  SS.getWithLocInContext(Context), ULE->getTemplateKeywordLoc(),
+                  nullptr, R.getLookupNameInfo(),
                   ULE->hasExplicitTemplateArgs() ? &TList : nullptr);
+          // HLSL Change End - This is a reference.
           CallsUndergoingInstantiation.back()->setCallee(DepExpr);
         } else {
           Diag(R.getNameLoc(), diagnostic) << Name;
@@ -2100,6 +2102,12 @@ recoverFromMSUnqualifiedLookup(Sema &S, ASTContext &Context,
   DB << NameInfo.getName() << RD;
 
   if (!ThisType.isNull()) {
+    // HLSL Change Begin - This code is broken because `this` is a reference in
+    // HLSL, but this code should also be unreachable.
+    assert(!S.getLangOpts().HLSL &&
+           "This should be unreachable in DXC because we don't enable the "
+           "MSCompat language feature.");
+    // HLSL Change End
     DB << FixItHint::CreateInsertion(Loc, "this->");
     return CXXDependentScopeMemberExpr::Create(
         Context, /*This=*/nullptr, ThisType, /*IsArrow=*/true,

tools/clang/lib/Sema/SemaExprCXX.cpp

@@ -970,7 +970,7 @@ ExprResult Sema::ActOnCXXThis(SourceLocation Loc) {
 
   CheckCXXThisCapture(Loc);
   // HLSL Change Starts - adjust this from T* to T&-like
-  if (getLangOpts().HLSL && ThisTy.getTypePtr()->isPointerType()) {
+  if (getLangOpts().HLSL) {
     return genereateHLSLThis(Loc, ThisTy, /*isImplicit=*/false);
   }
   // HLSL Change Ends
@@ -982,10 +982,10 @@ CXXThisExpr *Sema::genereateHLSLThis(SourceLocation Loc, QualType ThisType,
                                    bool isImplicit) {
   // Expressions cannot be of reference type - instead, they yield
   // an lvalue on the underlying type.
-  const Type *TypePtr = ThisType.getTypePtr();
-  CXXThisExpr *ResultExpr = new (Context) CXXThisExpr(
-      Loc, TypePtr->isPointerType() ? TypePtr->getPointeeType() : ThisType,
-      isImplicit);
+  if (ThisType->isPointerType() || ThisType->isReferenceType())
+    ThisType = ThisType->getPointeeType();
+  CXXThisExpr *ResultExpr =
+      new (Context) CXXThisExpr(Loc, ThisType, isImplicit);
   ResultExpr->setValueKind(ExprValueKind::VK_LValue);
   return ResultExpr;
 }

tools/clang/lib/Sema/SemaExprMember.cpp

@@ -507,6 +507,7 @@ bool Sema::CheckQualifiedMemberReference(Expr *BaseExpr,
                                          QualType BaseType,
                                          const CXXScopeSpec &SS,
                                          const LookupResult &R) {
+  BaseType = BaseType.getNonReferenceType(); // HLSL Change
   CXXRecordDecl *BaseRecord =
     cast_or_null<CXXRecordDecl>(computeDeclContext(BaseType));
   if (!BaseRecord) {
@@ -704,6 +705,7 @@ Sema::BuildMemberReferenceExpr(Expr *Base, QualType BaseType,
     TypoExpr *TE = nullptr;
     QualType RecordTy = BaseType;
     if (IsArrow) RecordTy = RecordTy->getAs<PointerType>()->getPointeeType();
+    RecordTy = RecordTy.getNonReferenceType(); // HLSL Change - implicit this is a reference.
     if (LookupMemberExprInRecord(*this, R, nullptr,
                                  RecordTy->getAs<RecordType>(), OpLoc, IsArrow,
                                  SS, TemplateArgs != nullptr, TE))
@@ -1051,7 +1053,7 @@ Sema::BuildMemberReferenceExpr(Expr *BaseExpr, QualType BaseExprType,
     CheckCXXThisCapture(Loc);
 
     // HLSL Change Starts - adjust this from T* to T&-like
-    if (getLangOpts().HLSL && BaseExprType->isPointerType())
+    if (getLangOpts().HLSL)
       BaseExpr = genereateHLSLThis(Loc, BaseExprType, /*isImplicit=*/true);
     else
       BaseExpr = new (Context) CXXThisExpr(Loc, BaseExprType,/*isImplicit=*/true);
@@ -1768,9 +1770,9 @@ Sema::BuildImplicitMemberExpr(const CXXScopeSpec &SS,
     if (SS.getRange().isValid())
       Loc = SS.getRange().getBegin();
     CheckCXXThisCapture(Loc);
-    if (getLangOpts().HLSL && ThisTy->isPointerType()) {
+    if (getLangOpts().HLSL) {
       baseExpr = genereateHLSLThis(Loc, ThisTy, /*isImplicit=*/true);
-      ThisTy = ThisTy->getAs<PointerType>()->getPointeeType();
+      ThisTy = ThisTy->getPointeeType();
     } else
       baseExpr = new (Context) CXXThisExpr(loc, ThisTy, /*isImplicit=*/true);
   }

tools/clang/lib/Sema/SemaOverload.cpp

@@ -4987,8 +4987,8 @@ Sema::PerformObjectArgumentInitialization(Expr *From,
                                           NamedDecl *FoundDecl,
                                           CXXMethodDecl *Method) {
   QualType FromRecordType, DestType;
-  QualType ImplicitParamRecordType  =
-    Method->getThisType(Context)->getAs<PointerType>()->getPointeeType();
+  // HLSL Change - this is a reference.
+  QualType ImplicitParamRecordType = Method->getThisObjectType(Context);
 
   Expr::Classification FromClassification;
   if (const PointerType *PT = From->getType()->getAs<PointerType>()) {

tools/clang/lib/Sema/SemaTemplate.cpp

@@ -420,10 +420,12 @@ Sema::ActOnDependentIdExpression(const CXXScopeSpec &SS,
     // perform the double-lookup check.
     NamedDecl *FirstQualifierInScope = nullptr;
 
+    // HLSL Change begin - This is a reference.
     return CXXDependentScopeMemberExpr::Create(
-        Context, /*This*/ nullptr, ThisType, /*IsArrow*/ true,
+        Context, /*This*/ nullptr, ThisType, /*IsArrow*/ !getLangOpts().HLSL,
         /*Op*/ SourceLocation(), SS.getWithLocInContext(Context), TemplateKWLoc,
         FirstQualifierInScope, NameInfo, TemplateArgs);
+    // HLSL Change end - This is a reference.
   }
 
   return BuildDependentDeclRefExpr(SS, TemplateKWLoc, NameInfo, TemplateArgs);

tools/clang/lib/Sema/TreeTransform.h

@@ -2344,7 +2344,7 @@ class TreeTransform {
                                 bool isImplicit) {
     getSema().CheckCXXThisCapture(ThisLoc);
     // HLSL Change Begin - adjust this from T* to T&-like
-    if (getSema().getLangOpts().HLSL && ThisType.getTypePtr()->isPointerType())
+    if (getSema().getLangOpts().HLSL)
       return getSema().genereateHLSLThis(ThisLoc, ThisType, isImplicit);
     // HLSL Change End - adjust this from T* to T&-like
     return new (getSema().Context) CXXThisExpr(ThisLoc, ThisType, isImplicit);
@@ -9777,7 +9777,7 @@ TreeTransform<Derived>::TransformCXXDependentScopeMemberExpr(
   } else {
     OldBase = nullptr;
     BaseType = getDerived().TransformType(E->getBaseType());
-    ObjectType = BaseType->getAs<PointerType>()->getPointeeType();
+    ObjectType = BaseType->getPointeeType();
   }
 
   // Transform the first part of the nested-name-specifier that qualifies

tools/clang/test/HLSLFileCheck/hlsl/classes/template_base_this.hlsl

@@ -0,0 +1,55 @@
+// RUN: %dxc -T lib_6_4 -HV 2021 %s -ast-dump | FileCheck %s -check-prefix=AST
+// RUN: %dxc -T lib_6_4 -HV 2021 %s -fcgl | FileCheck %s
+
+// This test verifies two things. First it verifies that the AST instantiates a
+// correct AST where the `CXXThisExpr` is an lvalue of type array_ext<float, 3>
+// rather than a pointer (as C++ would have).
+
+// Secondarily it verifies that the code geneariton for the `this` reference
+// correctly resolves to the base pointer and indexes off the base class member.
+
+// AST: ClassTemplateDecl {{.*}} array_ext
+// AST-NEXT: TemplateTypeParmDecl {{.*}} referenced typename T
+// AST-NEXT: NonTypeTemplateParmDecl {{.*}} referenced 'uint32_t':'unsigned int' N
+// AST-NEXT: CXXRecordDecl {{.*}} class array_ext definition
+// AST-NEXT: public 'array<T, N>'
+
+// AST: ClassTemplateSpecializationDecl {{.*}} class array_ext definition
+// AST: TemplateArgument type 'float'
+// AST-NEXT: TemplateArgument integral 3
+// AST-NEXT: CXXRecordDecl {{.*}} implicit class array_ext
+// AST-NEXT: CXXMethodDecl {{.*}} used test 'float ()'
+// AST-NEXT: CompoundStmt
+// AST-NEXT: ReturnStmt
+// AST-NEXT: ImplicitCastExpr {{.*}} 'float':'float' <LValueToRValue>
+// AST-NEXT: ArraySubscriptExpr {{.*}} 'float':'float' lvalue
+
+// Note: the implicit LValueToRvalue casts below are nonsensical as noted by them
+// producing lvalues. This test verifies them only to ensure the correct ASTs
+// around the casts. The casts themselves might be removed or changed in a
+// future change.
+
+// AST-NEXT: ImplicitCastExpr {{.*}} 'float [3]' <LValueToRValue>
+// AST-NEXT: MemberExpr {{.*}} 'float [3]' lvalue .mArr
+// AST-NEXT: ImplicitCastExpr {{.*}} 'array<float, 3U>':'array<float, 3>' lvalue <UncheckedDerivedToBase (array)>
+// AST-NEXT: CXXThisExpr {{.* }}'array_ext<float, 3>' lvalue this
+// AST-NEXT: IntegerLiteral {{.*}} 'literal int' 0
+
+template <typename T, uint32_t N> class array { T mArr[N]; };
+
+template <typename T, uint32_t N> class array_ext : array<T, N> {
+  float test() { return array<T, N>::mArr[0]; }
+};
+
+// CHECK: define linkonce_odr float @"\01?test@?$array_ext@{{.*}}"(%"class.array_ext<float, 3>"* [[this:%.+]])
+// CHECK: [[basePtr:%[0-9]+]] = bitcast %"class.array_ext<float, 3>"* [[this]] to %"class.array<float, 3>"*
+// CHECK: [[mArr:%.+]] = getelementptr inbounds %"class.array<float, 3>", %"class.array<float, 3>"* [[basePtr]], i32 0, i32 0
+// CHECK: [[elemPtr:%.+]] = getelementptr inbounds [3 x float], [3 x float]* [[mArr]], i32 0, i32 0
+// CHECK: [[Val:%.+]] = load float, float* [[elemPtr]]
+// CHECK: ret float [[Val]]
+
+// This function only exists to force instantiation of the template.
+float fn() {
+  array_ext<float, 3> arr1;
+  return arr1.test();
+}

tools/clang/test/HLSLFileCheck/hlsl/classes/this_reference_2018.hlsl

@@ -0,0 +1,54 @@
+// RUN: %dxc -T lib_6_6 %s -HV 2018 -ast-dump | FileCheck %s -check-prefix=AST
+// RUN: %dxc -T lib_6_6 %s -HV 2018 -fcgl | FileCheck %s
+// RUN: %dxc -T lib_6_6 %s -HV 2021 -ast-dump | FileCheck %s -check-prefix=AST
+// RUN: %dxc -T lib_6_6 %s -HV 2021 -fcgl | FileCheck %s
+
+// This test verifies two things, and it verifies them each under both HLSL 2018
+// and HLSL 2021 language modes. The behavior between the two modes should not
+// differ.
+
+// The first thing this verifies is that the AST formulation for
+// `array_ext::test` uses the `this` reference as an lvalue of type `array_ext`
+// rather than a pointer (as C++ would).
+
+// The second part of this test is to verify the code generation to verify that
+// the base class address is resolved and that the member is indexed off the
+// base class as expected.
+
+// AST: CXXRecordDecl {{.*}} referenced class array definition
+// AST-NEXT: CXXRecordDecl {{.*}} implicit class array
+// AST-NEXT: FieldDecl {{.*}} referenced mArr 'float [4]'
+// AST-NEXT: CXXRecordDecl {{.*}} class array_ext definition
+// AST-NEXT: public 'array'
+// AST-NEXT: CXXRecordDecl {{.*}} implicit class array_ext
+// AST-NEXT: CXXMethodDecl {{.*}} test 'float ()'
+// AST-NEXT: CompoundStmt
+// AST-NEXT: ReturnStmt
+// AST-NEXT: ImplicitCastExpr {{.*}} 'float' <LValueToRValue>
+// AST-NEXT: ArraySubscriptExpr {{.*}} 'float' lvalue
+// AST-NEXT: ImplicitCastExpr {{.*}} 'float [4]' <LValueToRValue>
+// AST-NEXT: MemberExpr {{.*}} 'float [4]' lvalue .mArr
+// AST-NEXT: ImplicitCastExpr {{.*}} 'array' lvalue <UncheckedDerivedToBase (array)>
+// AST-NEXT: CXXThisExpr {{.*}} 'array_ext' lvalue this
+// AST-NEXT: IntegerLiteral {{.*}} 'literal int' 0
+
+class array {
+  float mArr[4];
+};
+
+class array_ext : array {
+  float test() { return array::mArr[0]; }
+};
+
+// CHECK: define linkonce_odr float @"\01?test@array_ext@{{.*}}"(%class.array_ext* [[this:%.+]])
+// CHECK: [[basePtr:%[0-9]+]] = bitcast %class.array_ext* [[this]] to %class.array*
+// CHECK: [[mArr:%.+]] = getelementptr inbounds %class.array, %class.array* [[basePtr]], i32 0, i32 0
+// CHECK: [[elemPtr:%.+]] = getelementptr inbounds [4 x float], [4 x float]* [[mArr]], i32 0, i32 0
+// CHECK: [[Val:%.+]] = load float, float* [[elemPtr]]
+// CHECK: ret float [[Val]]
+
+// This function only exists to force generation of the internal methods
+float fn() {
+  array_ext arr1;
+  return arr1.test();
+}