Skip to content

[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

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

bogner
Copy link
Contributor

@bogner bogner commented Nov 18, 2024

This is a kind of StructuredBuffer, so it should be "Raw" and not "Typed".

This is a kind of StructuredBuffer, so it should be "Raw" and not "Typed".
@llvmbot 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 Nov 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 18, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-hlsl

Author: Justin Bogner (bogner)

Changes

This 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:

  • (modified) clang/lib/Sema/HLSLExternalSemaSource.cpp (+1-1)
  • (modified) clang/test/AST/HLSL/RasterizerOrderedStructuredBuffer-AST.hlsl (+2-2)
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

Copy link

github-actions bot commented Nov 18, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Comment on lines -483 to +484
ResourceKind::TypedBuffer,
/*IsROV=*/false, /*RawBuffer=*/false)
ResourceKind::TypedBuffer, /*IsROV=*/false,
/*RawBuffer=*/false)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Comment on lines +546 to +547
setupBufferType(Decl, *SemaPtr, ResourceClass::UAV, ResourceKind::RawBuffer,
/*IsROV=*/true, /*RawBuffer=*/true)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@bogner bogner merged commit 4bd982d into llvm:main Nov 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category HLSL HLSL Language Support
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants