[HLSL] Add testing for space parameter on global constants #106782

bob80905 · 2024-08-30T19:11:26Z

The space parameter in the register binding annotation may not be used for global constants. There was previously no diagnostic emitted when this case occurred. This PR adds a diagnostic when this case occurs, and tests these cases.
A new file was made to specifically test the space parameter, so some cases in \clang\test\SemaHLSL\resource_binding_attr_error.hlsl were moved over to this new file.
Fixes #104521

llvmbot · 2024-08-30T19:11:54Z

@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Joshua Batista (bob80905)

Changes

The space parameter in the register binding annotation may not be used for global constants. There was previously no diagnostic emitted when this case occurred. This PR adds a diagnostic when this case occurs, and tests these cases.
Fixes #104521

Full diff: https://github.com/llvm/llvm-project/pull/106782.diff

5 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1)
(modified) clang/lib/Sema/SemaHLSL.cpp (+12-4)
(modified) clang/test/SemaHLSL/resource_binding_attr_error.hlsl (+3-14)
(modified) clang/test/SemaHLSL/resource_binding_attr_error_basic.hlsl (+12-9)
(added) clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl (+25)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index edf22b909c4d57..e099cb954cac75 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12357,6 +12357,7 @@ def warn_hlsl_deprecated_register_type_b: Warning<"binding type 'b' only applies
 def warn_hlsl_deprecated_register_type_i: Warning<"binding type 'i' ignored. The 'integer constant' binding type is no longer supported">, InGroup<LegacyConstantRegisterBinding>, DefaultError;
 def err_hlsl_unsupported_register_number : Error<"register number should be an integer">;
 def err_hlsl_expected_space : Error<"invalid space specifier '%0' used; expected 'space' followed by an integer, like space1">;
+def err_hlsl_space_on_global_constant : Error<"register space cannot be specified on global constants">;
 def warn_hlsl_packoffset_mix : Warning<"cannot mix packoffset elements with nonpackoffset elements in a cbuffer">,
     InGroup<HLSLMixPackOffset>;
 def err_hlsl_packoffset_overlap : Error<"packoffset overlap between %0, %1">;
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index 714e8f5cfa9926..61bed66445dc78 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -722,7 +722,8 @@ static void ValidateMultipleRegisterAnnotations(Sema &S, Decl *TheDecl,
 }
 
 static void DiagnoseHLSLRegisterAttribute(Sema &S, SourceLocation &ArgLoc,
-                                          Decl *TheDecl, RegisterType regType) {
+                                          Decl *TheDecl, RegisterType regType,
+                                          StringRef SpaceNum) {
 
   // Samplers, UAVs, and SRVs are VarDecl types
   VarDecl *TheVarDecl = dyn_cast<VarDecl>(TheDecl);
@@ -795,6 +796,9 @@ static void DiagnoseHLSLRegisterAttribute(Sema &S, SourceLocation &ArgLoc,
         S.Diag(ArgLoc, diag::warn_hlsl_deprecated_register_type_b);
       else if (regType != RegisterType::C)
         S.Diag(ArgLoc, diag::err_hlsl_binding_type_mismatch) << regTypeNum;
+      // non-zero SpaceNum cannot be specified for global constants
+      if (SpaceNum != "0")
+        S.Diag(ArgLoc, diag::err_hlsl_space_on_global_constant);
       return;
     }
 
@@ -817,8 +821,12 @@ static void DiagnoseHLSLRegisterAttribute(Sema &S, SourceLocation &ArgLoc,
       S.Diag(TheDecl->getLocation(),
              diag::warn_hlsl_user_defined_type_missing_member)
           << regTypeNum;
-
-    return;
+    // non-zero SpaceNum cannot be specified for global constants
+    if (!isDeclaredWithinCOrTBuffer(TheDecl)) {
+      if (SpaceNum != "0")
+        S.Diag(ArgLoc, diag::err_hlsl_space_on_global_constant);
+      return;
+    }
   }
 }
 
@@ -891,7 +899,7 @@ void SemaHLSL::handleResourceBindingAttr(Decl *TheDecl, const ParsedAttr &AL) {
     return;
   }
 
-  DiagnoseHLSLRegisterAttribute(SemaRef, ArgLoc, TheDecl, regType);
+  DiagnoseHLSLRegisterAttribute(SemaRef, ArgLoc, TheDecl, regType, SpaceNum);
 
   HLSLResourceBindingAttr *NewAttr =
       HLSLResourceBindingAttr::Create(getASTContext(), Slot, Space, AL);
diff --git a/clang/test/SemaHLSL/resource_binding_attr_error.hlsl b/clang/test/SemaHLSL/resource_binding_attr_error.hlsl
index 6a0b5956545dd8..823b75f14d1957 100644
--- a/clang/test/SemaHLSL/resource_binding_attr_error.hlsl
+++ b/clang/test/SemaHLSL/resource_binding_attr_error.hlsl
@@ -13,16 +13,9 @@ float a : register(c0);
 cbuffer b : register(i0) {
 
 }
-// expected-error@+1 {{invalid space specifier 's2' used; expected 'space' followed by an integer, like space1}}
-cbuffer c : register(b0, s2) {
 
-}
 // expected-error@+1 {{register number should be an integer}}
-cbuffer d : register(bf, s2) {
-
-}
-// expected-error@+1 {{invalid space specifier 'spaces' used; expected 'space' followed by an integer, like space1}}
-cbuffer e : register(b2, spaces) {
+cbuffer c : register(bf, s2) {
 
 }
 
@@ -35,9 +28,8 @@ cbuffer B : register(space1) {}
 // expected-error@+1 {{wrong argument format for hlsl attribute, use b2 instead}}
 cbuffer C : register(b 2) {}
 
-// expected-error@+2 {{wrong argument format for hlsl attribute, use b2 instead}}
-// expected-error@+1 {{wrong argument format for hlsl attribute, use space3 instead}}
-cbuffer D : register(b 2, space 3) {}
+// expected-error@+1 {{wrong argument format for hlsl attribute, use b2 instead}}
+cbuffer D : register(b 2, space3) {}
 
 // expected-error@+1 {{'register' attribute only applies to cbuffer/tbuffer and external global variables}}
 static MyTemplatedSRV<float> U : register(u5);
@@ -61,9 +53,6 @@ void foo2() {
   extern MyTemplatedSRV<float> U2 : register(u5);
 }
 
-// expected-error@+1 {{binding type 'u' only applies to UAV resources}}
-float b : register(u0, space1);
-
 // expected-error@+1 {{'register' attribute only applies to cbuffer/tbuffer and external global variables}}
 void bar(MyTemplatedSRV<float> U : register(u3)) {
 
diff --git a/clang/test/SemaHLSL/resource_binding_attr_error_basic.hlsl b/clang/test/SemaHLSL/resource_binding_attr_error_basic.hlsl
index 0a547ed66af0a2..760c057630a7fa 100644
--- a/clang/test/SemaHLSL/resource_binding_attr_error_basic.hlsl
+++ b/clang/test/SemaHLSL/resource_binding_attr_error_basic.hlsl
@@ -3,37 +3,40 @@
 // expected-error@+1{{binding type 't' only applies to SRV resources}}
 float f1 : register(t0);
 
-
-float f2 : register(c0);
+// expected-error@+1 {{binding type 'u' only applies to UAV resources}}
+float f2 : register(u0);
 
 // expected-error@+1{{binding type 'b' only applies to constant buffers. The 'bool constant' binding type is no longer supported}}
 float f3 : register(b9);
 
+// expected-error@+1 {{binding type 's' only applies to sampler state}}
+float f4 : register(s0);
+
 // expected-error@+1{{binding type 'i' ignored. The 'integer constant' binding type is no longer supported}}
-float f4 : register(i9);
+float f5 : register(i9);
 
 // expected-error@+1{{binding type 'x' is invalid}}
-float f5 : register(x9);
+float f6 : register(x9);
 
 cbuffer g_cbuffer1 {
 // expected-error@+1{{binding type 'c' ignored in buffer declaration. Did you mean 'packoffset'?}}
-    float f6 : register(c2);
+    float f7 : register(c2);
 };
 
 tbuffer g_tbuffer1 {
 // expected-error@+1{{binding type 'c' ignored in buffer declaration. Did you mean 'packoffset'?}}
-    float f7 : register(c2);
+    float f8 : register(c2);
 };
 
 cbuffer g_cbuffer2 {
 // expected-error@+1{{binding type 'b' only applies to constant buffer resources}}
-    float f8 : register(b2);
+    float f9 : register(b2);
 };
 
 tbuffer g_tbuffer2 {
 // expected-error@+1{{binding type 'i' ignored. The 'integer constant' binding type is no longer supported}}
-    float f9 : register(i2);
+    float f10 : register(i2);
 };
 
 // expected-error@+1{{binding type 'c' only applies to numeric variables in the global scope}}
-RWBuffer<float> f10 : register(c3);
+RWBuffer<float> f11 : register(c3);
diff --git a/clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl b/clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl
new file mode 100644
index 00000000000000..2f1b19e1bfc696
--- /dev/null
+++ b/clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify
+
+
+// expected-error@+1 {{invalid space specifier 's2' used; expected 'space' followed by an integer, like space1}}
+cbuffer a : register(b0, s2) {
+
+}
+
+// expected-error@+1 {{invalid space specifier 'spaces' used; expected 'space' followed by an integer, like space1}}
+cbuffer b : register(b2, spaces) {
+
+}
+
+// expected-error@+1 {{wrong argument format for hlsl attribute, use space3 instead}}
+cbuffer c : register(b2, space 3) {}
+
+// expected-error@+1 {{register space cannot be specified on global constants}}
+int d : register(c2, space3);
+
+struct S {
+    RWBuffer<int> a;
+};
+
+// expected-error@+1 {{register space cannot be specified on global constants}}
+S thing : register(u2, space2);
\ No newline at end of file

clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl

clang/lib/Sema/SemaHLSL.cpp

clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl

clang/lib/Sema/SemaHLSL.cpp

damyanp

LGTM - I think one of the test cases may be redundant (or in the wrong file).

damyanp · 2024-09-13T01:06:31Z

clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl

+cbuffer cbuf2 {
+    struct x {
+        // expected-error@+1 {{'register' attribute only applies to cbuffer/tbuffer and external global variables}}
+        RWBuffer<int> E : register(u2, space3);
+    };
+}


Presumably we already have a test for this case somewhere else, since the error here is related to the presence of a register attribute at all and not due to register space being there.

I thought to add this to show why the !isDeclaredWithinCOrTBuffer call is necessary. It prevents the second diagnostic about the space parameter from being emitted. I don't believe there is a case like this that uses the space parameter.

Maybe a comment explaining that this test case is explicitly validating that an error isn't emitted would help understand that?

python3kgae · 2024-09-13T14:47:55Z

clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl

+};
+
+cbuffer cbuf3 {
+  MyStruct E : register(u2, space3);


This will hit error in dxc

invalid register specification, expected 'b', 'c', or 'i' binding

Compilation succeeds with -T lib_6_3 on godbolt.
I think we expect this to succeed in clang.

Sharing the godbolt links to the case where @python3kgae hit the error and @bob80905 doesn't hit the error would probably help the discussion here.

https://godbolt.org/z/7axojs5hP

https://godbolt.org/z/ezj1sc3sx - it looks like DXC doesn't actually get the binding right in this case:

; Name Type Format Dim ID HLSL Bind Count ; ------------------------------ ---------- ------- ----------- ------- -------------- ------ ; E.E UAV i32 buf U0 u4294967295 1

Also I think that the case we're discussing:

struct MyStruct { RWBuffer<int> E; }; cbuffer cbuf3 { MyStruct E : register(u2, space3); }

We should consider as equivalent to:

struct MyStruct { RWBuffer<int> E; }; MyStruct E : register(u2, space3);

And I think that we said that this would report a "can't use space on globals" error?

https://godbolt.org/z/G7cKdP3hY
cbuffer cbuf3 {
float a : register(u2, space3);
}

And I think that we said that this would report a "can't use space on globals" error?

I would argue those 2 cases are not equivalent, because one register annotation is not in the global scope, and the other is. So, the second case would emit an error about the annotation not being in the global scope, and the first would not.
I've explicitly added the second case along with the expected error for clarity.

https://godbolt.org/z/G7cKdP3hY cbuffer cbuf3 { float a : register(u2, space3); }

I agree that the code you provided should emit an error, I'll add your example to the test.

And I think that we said that this would report a "can't use space on globals" error?

I would argue those 2 cases are not equivalent, because one register annotation is not in the global scope, and the other is. So, the second case would emit an error about the annotation not being in the global scope, and the first would not. I've explicitly added the second case along with the expected error for clarity.

I disagree with this - we could file an issue on this and follow up since this change has other worthy things in it. Since DXC fails to do the right thing in this case it is arguably more a bug in DXC that it doesn't detect the error here.

Or we could open a can of worms and allow spaces on globals like this, but when we discussed this previously we decided to just match DXC's rules rather than try and make new ones.

damyanp · 2024-09-13T16:12:30Z

clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl

+};
+
+cbuffer cbuf3 {
+  MyStruct E : register(u2, space3);


https://godbolt.org/z/ezj1sc3sx - it looks like DXC doesn't actually get the binding right in this case:

; Name Type Format Dim ID HLSL Bind Count ; ------------------------------ ---------- ------- ----------- ------- -------------- ------ ; E.E UAV i32 buf U0 u4294967295 1

Also I think that the case we're discussing:

struct MyStruct { RWBuffer<int> E; }; cbuffer cbuf3 { MyStruct E : register(u2, space3); }

We should consider as equivalent to:

struct MyStruct { RWBuffer<int> E; }; MyStruct E : register(u2, space3);

And I think that we said that this would report a "can't use space on globals" error?

damyanp

I'm marking this as approved - I think that there's some cleanup and rearranging of the tests here that would improve this change.

It might be worth trying to get a review from someone who has more experience of testing strategies for these sorts of diagnostics than I do.

damyanp · 2024-09-16T18:13:17Z

clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl

+struct S {
+    RWBuffer<int> a;
+};


What is this for?

damyanp · 2024-09-16T18:16:59Z

clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl

@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify


This file seems to now mostly be testing non-error cases of specifying a space for registers. I think it'd probably be better to just merge these into the other tests we have.

Specifying a register space isn't some bit special thing, it's part of the normal flow we should consider when validating that, say, u type registers work with particular types of resource.

I will at least say it's simpler for a distinct file to test this, since we test multiple resource types (UDTs, basic, resources) within different scopes.

The space parameter in the register binding annotation may not be used for global constants. There was previously no diagnostic emitted when this case occurred. This PR adds a diagnostic when this case occurs, and tests these cases. A new file was made to specifically test the space parameter, so some cases in `\clang\test\SemaHLSL\resource_binding_attr_error.hlsl` were moved over to this new file. Fixes llvm#104521

add diag and testing for space and global constants

99408f3

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support labels Aug 30, 2024

add newline

9fb3c96

damyanp requested changes Aug 30, 2024

View reviewed changes

clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl Outdated Show resolved Hide resolved

clang/lib/Sema/SemaHLSL.cpp Outdated Show resolved Hide resolved

clang/test/SemaHLSL/resource_binding_attr_error_space.hlsl Show resolved Hide resolved

detect space param properly, add more tests

59218ac

damyanp requested changes Aug 31, 2024

View reviewed changes

clang/lib/Sema/SemaHLSL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaHLSL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaHLSL.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaHLSL.cpp Outdated Show resolved Hide resolved

bob80905 self-assigned this Sep 11, 2024

address damyan, simplify logic

1c1943d

damyanp approved these changes Sep 13, 2024

View reviewed changes

python3kgae reviewed Sep 13, 2024

View reviewed changes

Merge branch 'main' into test_space_register_binding

95287e6

damyanp requested changes Sep 13, 2024

View reviewed changes

bob80905 added 2 commits September 13, 2024 09:38

add more tests

83cc59d

no error on UDTs

35aa6d4

damyanp approved these changes Sep 16, 2024

View reviewed changes

remove struct unused

bd70840

python3kgae approved these changes Sep 17, 2024

View reviewed changes

bob80905 merged commit 905de9b into llvm:main Sep 17, 2024
8 checks passed

		@@ -0,0 +1,66 @@
		// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -o - -fsyntax-only %s -verify

[HLSL] Add testing for space parameter on global constants #106782

[HLSL] Add testing for space parameter on global constants #106782

Uh oh!

Conversation

bob80905 commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damyanp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bob80905 commented Aug 30, 2024 •

edited

Loading

llvmbot commented Aug 30, 2024 •

edited

Loading

damyanp left a comment •

edited

Loading