[clang] [ParserHLSL] Attempt to parse HLSL annotations on Field Decls. (PR #96346)
Joshua Batista via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 26 12:51:44 PDT 2024
https://github.com/bob80905 updated https://github.com/llvm/llvm-project/pull/96346
>From c267be670adf7aac050484dc1b243aa0eff60b5f Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista at microsoft.com>
Date: Fri, 21 Jun 2024 11:25:22 -0700
Subject: [PATCH 1/6] parse hlsl annotations on struct, add test
---
clang/lib/Parse/ParseDeclCXX.cpp | 3 ++
.../hlsl_annotations_on_struct_members.hlsl | 30 +++++++++++++++++++
2 files changed, 33 insertions(+)
create mode 100644 clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index d02548f6441f9..c4a4657cbd04f 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -2646,6 +2646,9 @@ bool Parser::ParseCXXMemberDeclaratorBeforeInitializer(
else
DeclaratorInfo.SetIdentifier(nullptr, Tok.getLocation());
+ if (getLangOpts().HLSL)
+ MaybeParseHLSLAnnotations(DeclaratorInfo);
+
if (!DeclaratorInfo.isFunctionDeclarator() && TryConsumeToken(tok::colon)) {
assert(DeclaratorInfo.isPastIdentifier() &&
"don't know where identifier would go yet?");
diff --git a/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl b/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl
new file mode 100644
index 0000000000000..f1e258b0d853c
--- /dev/null
+++ b/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -o - %s -verify
+
+// previously, this test would result in an error shown below on the line that
+// declares variable a in struct Eg9:
+// error: use of undeclared identifier
+// 'SV_DispatchThreadID'
+// This is because the annotation is parsed as if it was a c++ bit field, and an identifier
+// that represents an integer is expected, but not found.
+
+// This test ensures that hlsl annotations are attempted to be parsed when parsing struct decls.
+// Ideally, we'd validate this behavior by ensuring the annotation is parsed and properly
+// attached as an attribute to the member in the struct in the AST. However, in this case
+// this can't happen presently because there are other issues with annotations on field decls.
+// This test just ensures we make progress by moving the validation error from the realm of
+// C++ and expecting bitfields, to HLSL and a specialized error for the recognized annotation.
+
+struct Eg9{
+// expected-error at +1{{attribute 'SV_DispatchThreadID' only applies to parameter}}
+ int a : SV_DispatchThreadID;
+};
+Eg9 e9;
+
+
+RWBuffer<int> In : register(u1);
+
+
+[numthreads(1,1,1)]
+void main() {
+ In[0] = e9.a;
+}
>From 5c98dd990b938258bd94057220c61e85c1333e85 Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista at microsoft.com>
Date: Fri, 21 Jun 2024 12:37:21 -0700
Subject: [PATCH 2/6] unconsume colon if bitfield detected, add bitfield test
---
clang/include/clang/Parse/Parser.h | 8 +++++---
clang/lib/Parse/ParseDeclCXX.cpp | 3 ++-
clang/lib/Parse/ParseHLSL.cpp | 8 +++++++-
clang/test/ParserHLSL/bitfields.hlsl | 18 ++++++++++++++++++
.../hlsl_annotations_on_struct_members.hlsl | 14 +-------------
5 files changed, 33 insertions(+), 18 deletions(-)
create mode 100644 clang/test/ParserHLSL/bitfields.hlsl
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 95c0655f9a214..dc154c93a3b44 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -3017,11 +3017,12 @@ class Parser : public CodeCompletionHandler {
const IdentifierInfo *EnclosingScope = nullptr);
void MaybeParseHLSLAnnotations(Declarator &D,
- SourceLocation *EndLoc = nullptr) {
+ SourceLocation *EndLoc = nullptr,
+ bool CouldBeBitField = false) {
assert(getLangOpts().HLSL && "MaybeParseHLSLAnnotations is for HLSL only");
if (Tok.is(tok::colon)) {
ParsedAttributes Attrs(AttrFactory);
- ParseHLSLAnnotations(Attrs, EndLoc);
+ ParseHLSLAnnotations(Attrs, EndLoc, CouldBeBitField);
D.takeAttributes(Attrs);
}
}
@@ -3034,7 +3035,8 @@ class Parser : public CodeCompletionHandler {
}
void ParseHLSLAnnotations(ParsedAttributes &Attrs,
- SourceLocation *EndLoc = nullptr);
+ SourceLocation *EndLoc = nullptr,
+ bool CouldBeBitField = false);
Decl *ParseHLSLBuffer(SourceLocation &DeclEnd);
void MaybeParseMicrosoftAttributes(ParsedAttributes &Attrs) {
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index c4a4657cbd04f..461dbb2743c76 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -2647,7 +2647,8 @@ bool Parser::ParseCXXMemberDeclaratorBeforeInitializer(
DeclaratorInfo.SetIdentifier(nullptr, Tok.getLocation());
if (getLangOpts().HLSL)
- MaybeParseHLSLAnnotations(DeclaratorInfo);
+ MaybeParseHLSLAnnotations(DeclaratorInfo, nullptr,
+ /*CouldBeBitField*/ true);
if (!DeclaratorInfo.isFunctionDeclarator() && TryConsumeToken(tok::colon)) {
assert(DeclaratorInfo.isPastIdentifier() &&
diff --git a/clang/lib/Parse/ParseHLSL.cpp b/clang/lib/Parse/ParseHLSL.cpp
index 4b72afe9986e5..b36ea4012c26e 100644
--- a/clang/lib/Parse/ParseHLSL.cpp
+++ b/clang/lib/Parse/ParseHLSL.cpp
@@ -119,9 +119,11 @@ static void fixSeparateAttrArgAndNumber(StringRef ArgStr, SourceLocation ArgLoc,
}
void Parser::ParseHLSLAnnotations(ParsedAttributes &Attrs,
- SourceLocation *EndLoc) {
+ SourceLocation *EndLoc,
+ bool CouldBeBitField) {
assert(Tok.is(tok::colon) && "Not a HLSL Annotation");
+ Token OldToken = Tok;
ConsumeToken();
IdentifierInfo *II = nullptr;
@@ -131,6 +133,10 @@ void Parser::ParseHLSLAnnotations(ParsedAttributes &Attrs,
II = Tok.getIdentifierInfo();
if (!II) {
+ if (CouldBeBitField) {
+ UnconsumeToken(OldToken);
+ return;
+ }
Diag(Tok.getLocation(), diag::err_expected_semantic_identifier);
return;
}
diff --git a/clang/test/ParserHLSL/bitfields.hlsl b/clang/test/ParserHLSL/bitfields.hlsl
new file mode 100644
index 0000000000000..abb3109c1d72a
--- /dev/null
+++ b/clang/test/ParserHLSL/bitfields.hlsl
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -o - %s -verify
+
+// expected-no-diagnostics
+
+struct MyBitFields {
+ unsigned int field1 : 3; // 3 bits for field1
+ unsigned int field2 : 4; // 4 bits for field2
+ int field3 : 5; // 5 bits for field3 (signed)
+};
+
+
+
+[numthreads(1,1,1)]
+void main() {
+ MyBitFields m;
+ m.field1 = 4;
+ m.field2 = m.field1*2;
+}
\ No newline at end of file
diff --git a/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl b/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl
index f1e258b0d853c..6132ffa0ad476 100644
--- a/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl
+++ b/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl
@@ -1,18 +1,6 @@
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -o - %s -verify
-// previously, this test would result in an error shown below on the line that
-// declares variable a in struct Eg9:
-// error: use of undeclared identifier
-// 'SV_DispatchThreadID'
-// This is because the annotation is parsed as if it was a c++ bit field, and an identifier
-// that represents an integer is expected, but not found.
-
-// This test ensures that hlsl annotations are attempted to be parsed when parsing struct decls.
-// Ideally, we'd validate this behavior by ensuring the annotation is parsed and properly
-// attached as an attribute to the member in the struct in the AST. However, in this case
-// this can't happen presently because there are other issues with annotations on field decls.
-// This test just ensures we make progress by moving the validation error from the realm of
-// C++ and expecting bitfields, to HLSL and a specialized error for the recognized annotation.
+// TODO: update once we handle annotations on struct fields
struct Eg9{
// expected-error at +1{{attribute 'SV_DispatchThreadID' only applies to parameter}}
>From 4b6e23eb6e558340744ba8b18011d1ce839741bb Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista at microsoft.com>
Date: Fri, 21 Jun 2024 13:35:20 -0700
Subject: [PATCH 3/6] update existing tests
---
clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl | 4 ++--
clang/test/SemaHLSL/resource_binding_attr_error.hlsl | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl b/clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl
index bf8027d029882..8752e97b53f61 100644
--- a/clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl
+++ b/clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl
@@ -23,8 +23,8 @@ void foo() {
}
struct ST2 {
-// expected-error at +1 {{use of undeclared identifier 'SV_DispatchThreadID'}}
+// expected-warning at +1 {{'SV_DispatchThreadID' attribute only applies to parameters and non-static data members}}
static uint X : SV_DispatchThreadID;
-// expected-error at +1 {{use of undeclared identifier 'SV_DispatchThreadID'}}
+// expected-error at +1 {{attribute 'SV_DispatchThreadID' only applies to parameter}}
uint s : SV_DispatchThreadID;
};
diff --git a/clang/test/SemaHLSL/resource_binding_attr_error.hlsl b/clang/test/SemaHLSL/resource_binding_attr_error.hlsl
index 95774472aaea0..2f8aa098db701 100644
--- a/clang/test/SemaHLSL/resource_binding_attr_error.hlsl
+++ b/clang/test/SemaHLSL/resource_binding_attr_error.hlsl
@@ -55,6 +55,6 @@ void bar(RWBuffer<float> U : register(u3)) {
struct S {
// FIXME: generate better error when support semantic on struct field.
// See https://github.com/llvm/llvm-project/issues/57889.
- // expected-error at +1 {{expected expression}}
+ // expected-warning at +1 {{'register' attribute only applies to cbuffer/tbuffer and external global variables}}
RWBuffer<float> U : register(u3);
};
>From 34d4b4d05159ff61f9c535730c98d8dc1f5e8aec Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista at microsoft.com>
Date: Fri, 21 Jun 2024 13:44:46 -0700
Subject: [PATCH 4/6] remove redundant HLSL mode check
---
clang/include/clang/Parse/Parser.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index dc154c93a3b44..6cb223a14014e 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -3030,7 +3030,7 @@ class Parser : public CodeCompletionHandler {
void MaybeParseHLSLAnnotations(ParsedAttributes &Attrs,
SourceLocation *EndLoc = nullptr) {
assert(getLangOpts().HLSL && "MaybeParseHLSLAnnotations is for HLSL only");
- if (getLangOpts().HLSL && Tok.is(tok::colon))
+ if (Tok.is(tok::colon))
ParseHLSLAnnotations(Attrs, EndLoc);
}
>From 75263493c402976a2d71d2353ea8f25705aa1d49 Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista at microsoft.com>
Date: Wed, 26 Jun 2024 12:01:21 -0700
Subject: [PATCH 5/6] add AST tests in both tests, remove diagnostic for
SV_DispatchGrid
---
clang/lib/Sema/SemaHLSL.cpp | 7 +-----
clang/test/ParserHLSL/bitfields.hlsl | 23 +++++++++++++++----
.../hlsl_annotations_on_struct_members.hlsl | 11 +++++----
3 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index cc9c259858148..adc5e849c8b15 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -358,12 +358,7 @@ static bool isLegalTypeForHLSLSV_DispatchThreadID(QualType T) {
void SemaHLSL::handleSV_DispatchThreadIDAttr(Decl *D, const ParsedAttr &AL) {
// FIXME: support semantic on field.
- // See https://github.com/llvm/llvm-project/issues/57889.
- if (isa<FieldDecl>(D)) {
- Diag(AL.getLoc(), diag::err_hlsl_attr_invalid_ast_node)
- << AL << "parameter";
- return;
- }
+ // See https://github.com/llvm/llvm-project/issues/57889.
auto *VD = cast<ValueDecl>(D);
if (!isLegalTypeForHLSLSV_DispatchThreadID(VD->getType())) {
diff --git a/clang/test/ParserHLSL/bitfields.hlsl b/clang/test/ParserHLSL/bitfields.hlsl
index abb3109c1d72a..07a2dbd364172 100644
--- a/clang/test/ParserHLSL/bitfields.hlsl
+++ b/clang/test/ParserHLSL/bitfields.hlsl
@@ -1,11 +1,24 @@
-// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -o - %s -verify
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -ast-dump -x hlsl -o - %s | Filecheck %s
-// expected-no-diagnostics
struct MyBitFields {
- unsigned int field1 : 3; // 3 bits for field1
- unsigned int field2 : 4; // 4 bits for field2
- int field3 : 5; // 5 bits for field3 (signed)
+ // CHECK: FieldDecl 0x{{[0-9a-f]+}} <line:9:3, col:25> col:16 referenced field1 'unsigned int'
+ // CHECK:-ConstantExpr 0x{{[0-9a-f]+}} <col:25> 'int'
+ // CHECK:-value: Int 3
+ // CHECK:-IntegerLiteral 0x{{[0-9a-f]+}} <col:25> 'int' 3
+ unsigned int field1 : 3; // 3 bits for field1
+
+ // CHECK:FieldDecl 0x{{[0-9a-f]+}} <line:15:3, col:25> col:16 referenced field2 'unsigned int'
+ // CHECK:-ConstantExpr 0x{{[0-9a-f]+}} <col:25> 'int'
+ // CHECK:-value: Int 4
+ // CHECK:-IntegerLiteral 0x{{[0-9a-f]+}} <col:25> 'int' 4
+ unsigned int field2 : 4; // 4 bits for field2
+
+ // CHECK:FieldDecl 0x{{[0-9a-f]+}} <line:21:3, col:16> col:7 field3 'int'
+ // CHECK:-ConstantExpr 0x{{[0-9a-f]+}} <col:16> 'int'
+ // CHECK:-value: Int 5
+ // CHECK:-IntegerLiteral 0x{{[0-9a-f]+}} <col:16> 'int' 5
+ int field3 : 5; // 5 bits for field3 (signed)
};
diff --git a/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl b/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl
index 6132ffa0ad476..2eebc920388b5 100644
--- a/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl
+++ b/clang/test/ParserHLSL/hlsl_annotations_on_struct_members.hlsl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -o - %s -verify
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -ast-dump -o - %s | FileCheck %s
-// TODO: update once we handle annotations on struct fields
+// tests that hlsl annotations are properly parsed when applied on field decls,
+// and that the annotation gets properly placed on the AST.
struct Eg9{
-// expected-error at +1{{attribute 'SV_DispatchThreadID' only applies to parameter}}
- int a : SV_DispatchThreadID;
+ // CHECK: CXXRecordDecl 0x{{[0-9a-f]+}} <col:1, col:8> col:8 implicit struct Eg9
+ // CHECK: FieldDecl 0x{{[0-9a-f]+}} <line:10:3, col:16> col:16 referenced a 'unsigned int'
+ // CHECK: -HLSLSV_DispatchThreadIDAttr 0x{{[0-9a-f]+}} <col:20>
+ unsigned int a : SV_DispatchThreadID;
};
Eg9 e9;
>From 3940d2626cabeb301b634bb38211301bec7b8853 Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista at microsoft.com>
Date: Wed, 26 Jun 2024 12:51:24 -0700
Subject: [PATCH 6/6] update failing test, remove fixme
---
clang/lib/Sema/SemaHLSL.cpp | 5 +----
clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl | 1 -
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index adc5e849c8b15..9e58dd43ce0b4 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -356,10 +356,7 @@ static bool isLegalTypeForHLSLSV_DispatchThreadID(QualType T) {
return true;
}
-void SemaHLSL::handleSV_DispatchThreadIDAttr(Decl *D, const ParsedAttr &AL) {
- // FIXME: support semantic on field.
- // See https://github.com/llvm/llvm-project/issues/57889.
-
+void SemaHLSL::handleSV_DispatchThreadIDAttr(Decl *D, const ParsedAttr &AL) {
auto *VD = cast<ValueDecl>(D);
if (!isLegalTypeForHLSLSV_DispatchThreadID(VD->getType())) {
Diag(AL.getLoc(), diag::err_hlsl_attr_invalid_type)
diff --git a/clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl b/clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl
index 8752e97b53f61..bc3cf8bc51daf 100644
--- a/clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl
+++ b/clang/test/SemaHLSL/Semantics/invalid_entry_parameter.hlsl
@@ -25,6 +25,5 @@ void foo() {
struct ST2 {
// expected-warning at +1 {{'SV_DispatchThreadID' attribute only applies to parameters and non-static data members}}
static uint X : SV_DispatchThreadID;
-// expected-error at +1 {{attribute 'SV_DispatchThreadID' only applies to parameter}}
uint s : SV_DispatchThreadID;
};
More information about the cfe-commits
mailing list