Enhance `globallycoherent` mismatch diagnostics (#5121)

This change moves the `globallycoherent` mismatch diagnostics out of
code generation and into semantic analysis. Additionally the diagnostic
is enhanced to provide more information in the text about the mismatch,
and to fire in source locations more directly attributable to the
location of the mismatch.

This change also allows the `globallycoherent` annotation to apply to
functions, where it applies to the return type of the function, and
mismatches can be diagnosed on assignment of the return value and on
the return statements.

Fixes #4537.

tools/clang/include/clang/AST/HlslTypes.h

@@ -399,6 +399,7 @@ bool IsHLSLLineStreamType(clang::QualType type);
 bool IsHLSLTriangleStreamType(clang::QualType type);
 bool IsHLSLStreamOutputType(clang::QualType type);
 bool IsHLSLResourceType(clang::QualType type);
+bool IsHLSLDynamicResourceType(clang::QualType type);
 bool IsHLSLBufferViewType(clang::QualType type);
 bool IsHLSLStructuredBufferType(clang::QualType type);
 bool IsHLSLNumericOrAggregateOfNumericType(clang::QualType type);

tools/clang/include/clang/Basic/Attr.td

@@ -847,7 +847,7 @@ def HLSLTriangleAdj : InheritableAttr {
 
 def HLSLGloballyCoherent : InheritableAttr {
   let Spellings = [CXX11<"", "globallycoherent", 2015>];
-  let Subjects = SubjectList<[Var]>;
+  let Subjects = SubjectList<[Var, Function]>;
   let Documentation = [Undocumented];
 }
 

tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -7686,6 +7686,9 @@ def err_hlsl_incorrect_bind_semantic: Error< // Is an error in fxc in some cases
 def warn_hlsl_unary_negate_unsigned : Warning<
   "unary negate of unsigned value is still unsigned">,
   InGroup<Conversion>, DefaultWarn;
+def warn_hlsl_impcast_glc_mismatch : Warning<
+  "implicit conversion from %0 to %1 %select{loses|adds}2 globallycoherent annotation">,
+  InGroup<Conversion>, DefaultWarn;
 def warn_hlsl_narrowing : Warning<
   "conversion from larger type %0 to smaller type %1, possible loss of data">,
   InGroup<Conversion>, DefaultWarn;

tools/clang/include/clang/Sema/Sema.h

@@ -3794,6 +3794,9 @@ public:
   bool CheckHLSLUnaryExprOrTypeTraitOperand(QualType ExprType, SourceLocation Loc,
                                             UnaryExprOrTypeTrait ExprKind);
   void DiagnoseHLSLDeclAttr(const Decl *D, const Attr *A);
+  void DiagnoseGloballyCoherentMismatch(const Expr *SrcExpr,
+                                        QualType TargetType,
+                                        SourceLocation Loc);
   // HLSL Change Ends
 
   bool CheckUnaryExprOrTypeTraitOperand(Expr *E, UnaryExprOrTypeTrait ExprKind);

tools/clang/lib/AST/HlslTypes.cpp

@@ -571,6 +571,14 @@ bool IsHLSLResourceType(clang::QualType type) {
   return false;
 }
 
+bool IsHLSLDynamicResourceType(clang::QualType type) {
+  if (const RecordType *RT = type->getAs<RecordType>()) {
+    StringRef name = RT->getDecl()->getName();
+    return name == ".Resource";
+  }
+  return false;
+}
+
 bool IsHLSLBufferViewType(clang::QualType type) {
   if (const RecordType *RT = type->getAs<RecordType>()) {
     StringRef name = RT->getDecl()->getName();

tools/clang/lib/CodeGen/CGHLSLMS.cpp

@@ -2504,9 +2504,6 @@ static bool isGLCMismatch(QualType Ty0, QualType Ty1, const Expr *SrcExp,
     if (Cast->getCastKind() == CastKind::CK_FlatConversion)
       return false;
   }
-  unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Warning,
-                                          "global coherent mismatch");
-  Diags.Report(Loc, DiagID);
   return true;
 }
 

tools/clang/lib/Sema/SemaChecking.cpp

@@ -6771,6 +6771,9 @@ static void AnalyzeAssignment(Sema &S, BinaryOperator *E) {
   // Just recurse on the LHS.
   AnalyzeImplicitConversions(S, E->getLHS(), E->getOperatorLoc());
 
+  S.DiagnoseGloballyCoherentMismatch(E->getRHS(), E->getLHS()->getType(),
+                                     E->getOperatorLoc());
+
   // We want to recurse on the RHS as normal unless we're assigning to
   // a bitfield.
   if (FieldDecl *Bitfield = E->getLHS()->getSourceBitField()) {
@@ -6872,6 +6875,22 @@ static bool IsImplicitBoolFloatConversion(Sema &S, Expr *Ex, bool ToBool) {
 
 void CheckImplicitArgumentConversions(Sema &S, CallExpr *TheCall,
                                       SourceLocation CC) {
+  // HLSL Change Begin
+  if (FunctionDecl *FD = TheCall->getDirectCallee()) {
+    CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD);
+    unsigned ArgIdx = 0;
+    unsigned ParmIdx = 0;
+    if (MD && MD->isInstance())
+      ++ParmIdx;
+    for (; ArgIdx < TheCall->getNumArgs() && ParmIdx < FD->getNumParams();
+         ++ArgIdx, ++ParmIdx) {
+      ParmVarDecl *PD = FD->getParamDecl(ParmIdx);
+      Expr *CurrA = TheCall->getArg(ArgIdx);
+      S.DiagnoseGloballyCoherentMismatch(CurrA, PD->getType(), CC);
+    }
+  }
+  // HLSL CHange End
+
   unsigned NumArgs = TheCall->getNumArgs();
   for (unsigned i = 0; i < NumArgs; ++i) {
     Expr *CurrA = TheCall->getArg(i);

tools/clang/lib/Sema/SemaDecl.cpp

@@ -9157,6 +9157,13 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init,
   // Get the decls type and save a reference for later, since
   // CheckInitializerTypes may change it.
   QualType DclT = VDecl->getType(), SavT = DclT;
+
+  // HLSL Change begin
+  // When initializing an HLSL resource type we should diagnose mismatches in
+  // globally coherent annotations _unless_ the source is a dynamic resource
+  // placeholder type where we safely infer the globallycoherent annotaiton.
+  DiagnoseGloballyCoherentMismatch(Init, DclT, Init->getExprLoc());
+  // HLSL Change end
   
   // Expressions default to 'id' when we're in a debugger
   // and we are assigning it to a variable of Objective-C pointer type.

tools/clang/lib/Sema/SemaHLSL.cpp

@@ -12140,21 +12140,42 @@ void Sema::DiagnoseHLSLDeclAttr(const Decl *D, const Attr *A) {
   if (const HLSLGloballyCoherentAttr *HLSLGCAttr =
           dyn_cast<HLSLGloballyCoherentAttr>(A)) {
     const ValueDecl *TD = cast<ValueDecl>(D);
-    if (!TD->getType()->isDependentType()) {
-      QualType DeclType = TD->getType();
-      while (DeclType->isArrayType())
-        DeclType = QualType(DeclType->getArrayElementTypeNoTypeQual(), 0);
-      if (ExtSource->GetTypeObjectKind(DeclType) != AR_TOBJ_OBJECT ||
-          hlsl::GetResourceClassForType(getASTContext(), DeclType) !=
-              hlsl::DXIL::ResourceClass::UAV) {
-        Diag(A->getLocation(), diag::err_hlsl_varmodifierna)
-            << A << "non-UAV type";
-      }
+    if (TD->getType()->isDependentType())
+      return;
+    QualType DeclType = TD->getType();
+    if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(TD))
+      DeclType = FD->getReturnType();
+    while (DeclType->isArrayType())
+      DeclType = QualType(DeclType->getArrayElementTypeNoTypeQual(), 0);
+    if (ExtSource->GetTypeObjectKind(DeclType) != AR_TOBJ_OBJECT ||
+        hlsl::GetResourceClassForType(getASTContext(), DeclType) !=
+            hlsl::DXIL::ResourceClass::UAV) {
+      Diag(A->getLocation(), diag::err_hlsl_varmodifierna)
+          << A << "non-UAV type";
     }
     return;
   }
 }
 
+void Sema::DiagnoseGloballyCoherentMismatch(const Expr *SrcExpr,
+                                            QualType TargetType,
+                                            SourceLocation Loc) {
+  QualType SrcTy = SrcExpr->getType();
+  QualType DstTy = TargetType;
+  if (SrcTy->isArrayType() && DstTy->isArrayType()) {
+    SrcTy = QualType(SrcTy->getBaseElementTypeUnsafe(), 0);
+    DstTy = QualType(DstTy->getBaseElementTypeUnsafe(), 0);
+  }
+  if (hlsl::IsHLSLResourceType(DstTy) &&
+      !hlsl::IsHLSLDynamicResourceType(SrcTy)) {
+    bool SrcGL = hlsl::HasHLSLGloballyCoherent(SrcTy);
+    bool DstGL = hlsl::HasHLSLGloballyCoherent(DstTy);
+    if (SrcGL != DstGL)
+      Diag(Loc, diag::warn_hlsl_impcast_glc_mismatch)
+          << SrcExpr->getType() << TargetType << /*loses|adds*/ DstGL;
+  }
+}
+
 void hlsl::HandleDeclAttributeForHLSL(Sema &S, Decl *D, const AttributeList &A, bool& Handled)
 {
   DXASSERT_NOMSG(D != nullptr);

tools/clang/lib/Sema/SemaStmt.cpp

@@ -3182,6 +3182,11 @@ StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
     }
   }
 
+  // HLSL Change begin - Diagnose mismatched globallycoherent attrs on return.
+  if (RetValExp)
+    DiagnoseGloballyCoherentMismatch(RetValExp, FnRetType, ReturnLoc);
+  // HLSL Change end
+
   bool HasDependentReturnType = FnRetType->isDependentType();
 
   ReturnStmt *Result = nullptr;

tools/clang/test/HLSL/globallycoherent-errors.hlsl

@@ -8,6 +8,10 @@ globallycoherent float m; // expected-error {{'globallycoherent' is not a valid
 globallycoherent RWTexture2D<float> tex[12];
 globallycoherent RWTexture2D<float> texMD[12][12];
 
+globallycoherent float One() { // expected-error{{'globallycoherent' is not a valid modifier for a non-UAV type}}
+  return 1.0;
+}
+
  float4 main(uint2 a : A, uint2 b : B) : SV_Target
 {
   globallycoherent  RWTexture1D<float4> uav3 = uav1;

tools/clang/test/HLSL/globallycoherent-mismatch.hlsl

@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -fsyntax-only -ffreestanding -verify %s
+
+RWByteAddressBuffer NonGCBuf;
+globallycoherent RWByteAddressBuffer GCBuf;
+
+RWByteAddressBuffer NonGCBufArr[2];
+globallycoherent RWByteAddressBuffer GCBufArr[2];
+
+RWByteAddressBuffer NonGCBufMultiArr[2][2];
+globallycoherent RWByteAddressBuffer GCBufMultiArr[2][2];
+
+RWByteAddressBuffer getNonGCBuf() {
+  return NonGCBuf;
+}
+
+globallycoherent RWByteAddressBuffer getGCBuf() { 
+  return GCBuf;
+}
+
+RWByteAddressBuffer getNonGCBufArr() {
+  return NonGCBufArr[0];
+}
+
+globallycoherent RWByteAddressBuffer getGCBufArr() { 
+  return GCBufArr[0];
+}
+
+RWByteAddressBuffer getNonGCBufMultiArr() {
+  return NonGCBufMultiArr[0][0];
+}
+
+globallycoherent RWByteAddressBuffer getGCBufMultiArr() { 
+  return GCBufMultiArr[0][0];
+}
+
+RWByteAddressBuffer getNonGCGCBuf() {
+  return GCBuf; // expected-warning{{implicit conversion from 'globallycoherent RWByteAddressBuffer' to 'RWByteAddressBuffer' loses globallycoherent annotation}}
+}
+
+globallycoherent RWByteAddressBuffer getGCNonGCBuf() {
+  return NonGCBuf; // expected-warning{{implicit conversion from 'RWByteAddressBuffer' to 'globallycoherent RWByteAddressBuffer' adds globallycoherent annotation}}
+}
+
+RWByteAddressBuffer getNonGCGCBufArr() {
+  return GCBufArr[0]; // expected-warning{{implicit conversion from 'globallycoherent RWByteAddressBuffer' to 'RWByteAddressBuffer' loses globallycoherent annotation}}
+}
+
+globallycoherent RWByteAddressBuffer getGCNonGCBufArr() {
+  return NonGCBufArr[0]; // expected-warning{{implicit conversion from 'RWByteAddressBuffer' to 'globallycoherent RWByteAddressBuffer' adds globallycoherent annotation}}
+}
+
+RWByteAddressBuffer getNonGCGCBufMultiArr() {
+  return GCBufMultiArr[0][0]; // expected-warning{{implicit conversion from 'globallycoherent RWByteAddressBuffer' to 'RWByteAddressBuffer' loses globallycoherent annotation}}
+}
+
+globallycoherent RWByteAddressBuffer getGCNonGCBufMultiArr() {
+  return NonGCBufMultiArr[0][0]; // expected-warning{{implicit conversion from 'RWByteAddressBuffer' to 'globallycoherent RWByteAddressBuffer' adds globallycoherent annotation}}
+}
+
+void NonGCStore(RWByteAddressBuffer Buf) {
+  Buf.Store(0, 0);
+}
+
+void GCStore(globallycoherent RWByteAddressBuffer Buf) {
+  Buf.Store(0, 0);
+}
+
+
+void getNonGCBufPAram(inout globallycoherent RWByteAddressBuffer PGCBuf) {
+  PGCBuf = NonGCBuf; // expected-warning{{implicit conversion from 'RWByteAddressBuffer' to 'globallycoherent RWByteAddressBuffer __restrict' adds globallycoherent annotation}}
+}
+
+static globallycoherent RWByteAddressBuffer SGCBufArr[2] = NonGCBufArr; // expected-warning{{implicit conversion from 'RWByteAddressBuffer [2]' to 'globallycoherent RWByteAddressBuffer [2]' adds globallycoherent annotation}}
+static globallycoherent RWByteAddressBuffer SGCBufMultiArr0[2] = NonGCBufMultiArr[0]; // expected-warning{{implicit conversion from 'RWByteAddressBuffer [2]' to 'globallycoherent RWByteAddressBuffer [2]' adds globallycoherent annotation}}
+static globallycoherent RWByteAddressBuffer SGCBufMultiArr1[2][2] = NonGCBufMultiArr; // expected-warning{{implicit conversion from 'RWByteAddressBuffer [2][2]' to 'globallycoherent RWByteAddressBuffer [2][2]' adds globallycoherent annotation}}
+
+void getNonGCBufArrParam(inout globallycoherent RWByteAddressBuffer PGCBufArr[2]) {
+  PGCBufArr = NonGCBufArr; // expected-warning{{implicit conversion from 'RWByteAddressBuffer [2]' to 'globallycoherent RWByteAddressBuffer __restrict[2]' adds globallycoherent annotation}}
+}
+
+[numthreads(1, 1, 1)]
+void main()
+{
+  NonGCStore(NonGCBuf); // No diagnostic
+  GCStore(NonGCBuf); // expected-warning{{implicit conversion from 'RWByteAddressBuffer' to 'globallycoherent RWByteAddressBuffer' adds globallycoherent annotation}}
+  NonGCStore(GCBuf); // expected-warning{{implicit conversion from 'globallycoherent RWByteAddressBuffer' to 'RWByteAddressBuffer' loses globallycoherent annotation}}
+  GCStore(GCBuf); // No diagnostic
+
+  RWByteAddressBuffer NonGCCopyNonGC = NonGCBuf; // No diagnostic
+  RWByteAddressBuffer NonGCCopyGC = GCBuf; // expected-warning{{implicit conversion from 'globallycoherent RWByteAddressBuffer' to 'RWByteAddressBuffer' loses globallycoherent annotation}}
+
+  globallycoherent RWByteAddressBuffer GCCopyNonGC = NonGCBuf; // expected-warning{{implicit conversion from 'RWByteAddressBuffer' to 'globallycoherent RWByteAddressBuffer' adds globallycoherent annotation}}
+  globallycoherent RWByteAddressBuffer GCCopyGC = GCBuf; // No diagnostic
+
+  globallycoherent RWByteAddressBuffer GCCopyNonGCReturn = getNonGCBuf(); // expected-warning{{implicit conversion from 'RWByteAddressBuffer' to 'globallycoherent RWByteAddressBuffer' adds globallycoherent annotation}}
+
+  RWByteAddressBuffer NonGCCopyGCReturn = getGCBuf(); // expected-warning{{implicit conversion from 'globallycoherent RWByteAddressBuffer' to 'RWByteAddressBuffer' loses globallycoherent annotation}}
+
+  RWByteAddressBuffer NonGCCopyNonGC0 = NonGCBufArr[0]; // No diagnostic
+  RWByteAddressBuffer NonGCCopyGC0 = GCBufArr[0]; // expected-warning{{implicit conversion from 'globallycoherent RWByteAddressBuffer' to 'RWByteAddressBuffer' loses globallycoherent annotation}}
+
+  globallycoherent RWByteAddressBuffer GCCopyNonGC0 = NonGCBufArr[0]; // expected-warning{{implicit conversion from 'RWByteAddressBuffer' to 'globallycoherent RWByteAddressBuffer' adds globallycoherent annotation}}
+  globallycoherent RWByteAddressBuffer GCCopyGC0 = GCBufArr[0]; // No diagnostic
+}

tools/clang/test/HLSLFileCheck/hlsl/intrinsics/createHandleFromHeap/glc_merge.hlsl

@@ -1,13 +1,5 @@
 // RUN: %dxc -T cs_6_6 %s | %FileCheck %s
 
-// RUN: %dxc -T cs_6_6 %s | %FileCheck -input-file=stderr -check-prefix=CHK-WARNING %s
-
-// Make sure got warning.
-// CHK-WARNING:warning: global coherent mismatch
-// CHK-WARNING:Foo(Buffer0);
-// CHK-WARNING:warning: global coherent mismatch
-// CHK-WARNING:RWByteAddressBuffer Buffer1 = Buffer0;
-
 // Make sure only 1 annotate handle and mark glc.
 // CHECK:call %dx.types.Handle @dx.op.annotateHandle(i32 216, %dx.types.Handle %{{.*}}, %dx.types.ResourceProperties { i32 20491, i32 0 })
 // CHECK-NOT:call %dx.types.Handle @dx.op.annotateHandle(
@@ -33,4 +25,4 @@ void main()
 
     RWByteAddressBuffer Buffer1 = Buffer0;
     Buffer1.Store(0, 0);	// ; AnnotateHandle(res,props)  resource: RWByteAddressBuffer
-}
+}

tools/clang/test/HLSLFileCheck/hlsl/intrinsics/createHandleFromHeap/glc_merge2.hlsl

@@ -1,13 +1,5 @@
 // RUN: %dxc -T cs_6_6 %s | %FileCheck %s
 
-// RUN: %dxc -T cs_6_6 %s | %FileCheck -input-file=stderr -check-prefix=CHK-WARNING %s
-
-// Make sure got warning.
-// CHK-WARNING:warning: global coherent mismatch
-// CHK-WARNING:Foo(Buffer0);
-// CHK-WARNING:warning: global coherent mismatch
-// CHK-WARNING:RWByteAddressBuffer Buffer1 = Buffer0;
-
 // Make sure only 1 annotate handle and mark glc.
 // CHECK:call %dx.types.Handle @dx.op.annotateHandle(i32 216, %dx.types.Handle %{{.*}}, %dx.types.ResourceProperties { i32 20491, i32 0 })
 // CHECK-NOT:call %dx.types.Handle @dx.op.annotateHandle(
@@ -33,4 +25,4 @@ void main()
 
     RWByteAddressBuffer Buffer1 = Buffer0;
     Buffer1.Store(0, 0);	// ; AnnotateHandle(res,props)  resource: RWByteAddressBuffer
-}
+}

tools/clang/test/HLSLFileCheck/hlsl/intrinsics/createHandleFromHeap/glc_merge3.hlsl

@@ -1,9 +1,4 @@
 // RUN: %dxc -T ps_6_6 %s | %FileCheck %s
-// RUN: %dxc -T ps_6_6 %s | %FileCheck -input-file=stderr -check-prefix=CHK-WARNING %s
-
-// Make sure got warning.
-// CHK-WARNING:warning: global coherent mismatch
-// CHK-WARNING:return get(buf, i);
 
 // Make sure only 1 annotate handle and mark glc.
 // Make sure glc is set.
@@ -24,4 +19,4 @@ float4 main(uint i:I) : SV_Target {
 
   return get(buf, i);
 
-}
+}

tools/clang/unittests/HLSL/VerifierTest.cpp

@@ -105,6 +105,7 @@ public:
   TEST_METHOD(RunAtomicsOnBitfields)
   TEST_METHOD(RunUnboundedResourceArrays)
   TEST_METHOD(GloballyCoherentErrors)
+  TEST_METHOD(GloballyCoherentMismatch)
   TEST_METHOD(GloballyCoherentTemplateErrors)
   TEST_METHOD(RunBitFieldAnnotations)
   TEST_METHOD(RunUDTByteAddressBufferLoad)
@@ -463,6 +464,10 @@ TEST_F(VerifierTest, GloballyCoherentErrors) {
   CheckVerifiesHLSL(L"globallycoherent-errors.hlsl");
 }
 
+TEST_F(VerifierTest, GloballyCoherentMismatch) {
+  CheckVerifiesHLSL(L"globallycoherent-mismatch.hlsl");
+}
+
 TEST_F(VerifierTest, GloballyCoherentTemplateErrors) {
   CheckVerifiesHLSL(L"globallycoherent-template-errors.hlsl");
 }