-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[HLSL] Fix resource kind for RasterizerOrderedStructuredBuffer #116700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is a kind of StructuredBuffer, so it should be "Raw" and not "Typed".
@llvm/pr-subscribers-clang @llvm/pr-subscribers-hlsl Author: Justin Bogner (bogner) ChangesThis is a kind of StructuredBuffer, so it should be "Raw" and not "Typed". Full diff: https://github.com/llvm/llvm-project/pull/116700.diff 2 Files Affected:
diff --git a/clang/lib/Sema/HLSLExternalSemaSource.cpp b/clang/lib/Sema/HLSLExternalSemaSource.cpp
index cac15b974a276e..1013885928b1d8 100644
--- a/clang/lib/Sema/HLSLExternalSemaSource.cpp
+++ b/clang/lib/Sema/HLSLExternalSemaSource.cpp
@@ -548,7 +548,7 @@ void HLSLExternalSemaSource::defineHLSLTypesWithForwardDeclarations() {
.Record;
onCompletion(Decl, [this](CXXRecordDecl *Decl) {
setupBufferType(Decl, *SemaPtr, ResourceClass::UAV,
- ResourceKind::TypedBuffer, /*IsROV=*/true,
+ ResourceKind::RawBuffer, /*IsROV=*/true,
/*RawBuffer=*/true)
.addArraySubscriptOperators()
.completeDefinition();
diff --git a/clang/test/AST/HLSL/RasterizerOrderedStructuredBuffer-AST.hlsl b/clang/test/AST/HLSL/RasterizerOrderedStructuredBuffer-AST.hlsl
index f334e1bb6db3fc..eb8bfeffb481f1 100644
--- a/clang/test/AST/HLSL/RasterizerOrderedStructuredBuffer-AST.hlsl
+++ b/clang/test/AST/HLSL/RasterizerOrderedStructuredBuffer-AST.hlsl
@@ -35,7 +35,7 @@ RasterizerOrderedStructuredBuffer<int> Buffer;
// CHECK-SAME{LITERAL}: [[hlsl::is_rov]]
// CHECK-SAME{LITERAL}: [[hlsl::raw_buffer]]
// CHECK-SAME{LITERAL}: [[hlsl::contained_type(element_type)]]
-// CHECK-NEXT: HLSLResourceAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit TypedBuffer
+// CHECK-NEXT: HLSLResourceAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit RawBuffer
// CHECK: CXXMethodDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> operator[] 'element_type &const (unsigned int) const'
// CHECK-NEXT: ParmVarDecl 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> <invalid sloc> Idx 'unsigned int'
@@ -63,4 +63,4 @@ RasterizerOrderedStructuredBuffer<int> Buffer;
// CHECK-SAME{LITERAL}: [[hlsl::is_rov]]
// CHECK-SAME{LITERAL}: [[hlsl::raw_buffer]]
// CHECK-SAME{LITERAL}: [[hlsl::contained_type(int)]]
-// CHECK-NEXT: HLSLResourceAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit TypedBuffer
+// CHECK-NEXT: HLSLResourceAttr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> Implicit RawBuffer
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ResourceKind::TypedBuffer, | ||
/*IsROV=*/false, /*RawBuffer=*/false) | ||
ResourceKind::TypedBuffer, /*IsROV=*/false, | ||
/*RawBuffer=*/false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this reformatted? I think there's no change here other than formatting - I thought we had something that enforced clang-format conformance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-format
was complaining about the line I actually changed and I noticed that all of these looked funny. Seems like there's a "comment at the beginning of a line" heuristic that stops clang-format from messing with these, but if you collapse them to a single line and then run clang-format it does something pretty reasonable.
setupBufferType(Decl, *SemaPtr, ResourceClass::UAV, ResourceKind::RawBuffer, | ||
/*IsROV=*/true, /*RawBuffer=*/true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would we ever want RawBuffer to be false and ResourceKind to be something other than RawBuffer? Or are we going to be removing one of these in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResourceKind
won't be specified here in the future (See #104862), which will remove the redundancy. This change doesn't actually do much as is, but the inconsistency was confusing.
This is a kind of StructuredBuffer, so it should be "Raw" and not "Typed".