Allows Vulkan spec constants as attribute arguments. #7439

danbrown-amd · 2025-05-08T03:01:24Z

github-actions · 2025-05-08T03:02:18Z

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

tools/clang/lib/AST/ExprConstant.cpp

tools/clang/lib/SPIRV/SpirvEmitter.cpp

s-perron

I don't mind this change generally. It will be good for SPIR-V.

I don't know if this is complete yet (it is still a draft). I pointed out one issue with using LocalSize. You might also need to look at SpirvEmitter::addDerivativeGroupExecutionMode(). That function determines which derivative group execution mode to add based on the number of threas in the group. However, if you have a spec constant for the number of threads, we cannot do that.

…tributes).

danbrown-amd · 2025-06-17T04:59:44Z

Why was SpirvEmitter::addDerivativeGroupExecutionMode() written that way? It seems to me that it wouldn't be that difficult to provide enough context to find the original attribute in the AST so the method wouldn't need to scan the previously emitted instructions.

Of course, since there's no access to specializations at compile time, there's no way to ensure the best choice of execution mode with spec constants anyway. However, in some cases partial information would be sufficient to rule out one of them. If both are possible, maybe pick one and issue a warning?

s-perron · 2025-07-07T13:23:06Z

Why was SpirvEmitter::addDerivativeGroupExecutionMode() written that way?

It was implemented that way because that was the HLSL spec implemented for DXIL. See Thread Groups and Quads. I don't mind if we diverge from it when there are specialization constants. We just need to document it.

If both are possible, maybe pick one and issue a warning?

That is a reasonable idea. Longer term, we could try to add explicit attribute that apply to the entry point that would override the guess.

s-perron

LGTM, but requires some tests.

tools/clang/lib/SPIRV/SpirvEmitter.cpp

Also restores file inadvertently committed source file changed for debugging.

tools/clang/test/CodeGenSPIRV/vk.spec-constant.attributes.derivative-group.hlsl

llvm-beanz

I think there is a lot of missing test coverage here. You're making a bunch of attributes that had previously either required integer literal arguments or optional integer literal arguments now support constant expressions and specialization constants.

We really need test coverage for each attribute's change in behavior, and the error states. In particular it doesn't seem to me like you're actually handling the possible error of a non-constant expression being provided as a value. I think it just silently replaces the value with the default (which would be bad).

llvm-beanz · 2025-07-21T20:52:15Z

tools/clang/lib/CodeGen/CGHLSLMS.cpp

@@ -328,6 +328,19 @@ class CGMSHLSLRuntime : public CGHLSLRuntime {
 };
 } // namespace

+static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,
+                                   uint32_t defaultVal = 0) {


Suggested change

uint32_t defaultVal = 0) {

uint32_t defaultVal) {

Doesn't look like this is ever called without a default value.

I've changed this to use llvm::Optional so that an absent default indicates that expr should be non-null. (See note on SemaHLSL below.)

llvm-beanz · 2025-07-21T20:55:11Z

tools/clang/test/CodeGenSPIRV/vk.spec-constant.attributes.derivative-group.hlsl

+const uint NumThreadsY = 1;
+#endif
+
+[numthreads(NumThreadsX,NumThreadsY,1)]


Some other test cases that are probably interesting:

uint CBufferValue; [numthreads(CbufferValue, 1, 1)] // Should error void main() {}

const uint Eight = 8; [numthreads(Eight * 2, Eight, 1)] // Should work void main() {}

The latter won't work but would if declared static. Larger expressions incorporating spec constants don't work in the current version.

I've added preprocessor directives to vk.spec-constants.attributes.hlsl to enable removal of the spec constant designations to test for expected errors.

llvm-beanz · 2025-07-21T21:00:33Z

tools/clang/lib/Sema/SemaHLSL.cpp

+static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,
+                                   uint32_t defaultVal = 0) {


Suggested change

static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,

uint32_t defaultVal = 0) {

static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,

uint32_t defaultVal) {

It is really unfortunate to completely duplicate this code, and I don't think the two should be identical. In this version in Sema we should be surfacing an error if there is an expression that isn't a constant expression or spec constant.

Sema already seems to complain in all the relevant cases, though I have added some more diagnostics. Of the arguments allowed to be spec constants, the only optional one is the NodeId index, so maybe just return zero (possibly after issuing an error) whenever expr is null? For now the function works as described for the CGHLSLMS version.

llvm-beanz · 2025-07-21T21:01:50Z

tools/clang/lib/CodeGen/CGHLSLMS.cpp

+      return (uint32_t)apsInt.getSExtValue();
+    if (expr->isVulkanSpecConstantExpr(astContext, &apValue) && apValue.isInt())
+      return (uint32_t)apValue.getInt().getSExtValue();
+  }


Suggested change

}

llvm_unreachable("Expression must be a constant expression or spec constant")

}

We should be erroring in Sema if this isn't a constant expression or spec constant.

Changed as suggested.

llvm-beanz · 2025-07-21T21:04:15Z

tools/clang/lib/CodeGen/CGHLSLMS.cpp

@@ -328,6 +328,19 @@ class CGMSHLSLRuntime : public CGHLSLRuntime {
 };
 } // namespace

+static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,


You've got a fair amount of mixing coding style, and DXC is a bit inconsistent.

In general if you're adding code in a part of the codebase that isn't otherwise following a consistent style we follow the LLVM Coding Standards. LLVM uses CamelCase for variable names and type names (see: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).

github-project-automation bot added this to HLSL Roadmap May 8, 2025

github-project-automation bot moved this to New in HLSL Roadmap May 8, 2025

danbrown-amd force-pushed the attr-arg-consts branch 3 times, most recently from 9db3b6c to 243acbc Compare May 13, 2025 22:08

s-perron reviewed May 14, 2025

View reviewed changes

tools/clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved

s-perron reviewed May 14, 2025

View reviewed changes

tools/clang/lib/SPIRV/SpirvEmitter.cpp Outdated Show resolved Hide resolved

s-perron reviewed May 14, 2025

View reviewed changes

danbrown-amd mentioned this pull request May 21, 2025

[spirv] AMD work graphs extension #7353

Merged

danbrown-amd force-pushed the attr-arg-consts branch 2 times, most recently from 58352af to 357813d Compare June 12, 2025 02:30

[spirv] Allows spec constants as attribute arguments (for selected at…

8e898bf

…tributes).

danbrown-amd force-pushed the attr-arg-consts branch from 357813d to 8e898bf Compare June 16, 2025 22:05

Addresses PR comments.

27547de

danbrown-amd marked this pull request as ready for review June 19, 2025 18:27

danbrown-amd changed the title ~~Allows defined constants as attribute arguments.~~ Allows Vulkan spec constants as attribute arguments. Jun 26, 2025

s-perron reviewed Jul 7, 2025

View reviewed changes

tools/clang/lib/SPIRV/SpirvEmitter.cpp Show resolved Hide resolved

tools/clang/lib/SPIRV/SpirvEmitter.cpp Outdated Show resolved Hide resolved

tools/clang/lib/SPIRV/SpirvEmitter.cpp Show resolved Hide resolved

s-perron mentioned this pull request Jul 8, 2025

[SPIR-V] Allow thread group size to be specified with specialization constants #3092

Open

Addresses additional review comments.

a9e9d4b

Also restores file inadvertently committed source file changed for debugging.

s-perron approved these changes Jul 11, 2025

View reviewed changes

tools/clang/test/CodeGenSPIRV/vk.spec-constant.attributes.derivative-group.hlsl Show resolved Hide resolved

tools/clang/test/CodeGenSPIRV/vk.spec-constant.attributes.derivative-group.hlsl Show resolved Hide resolved

s-perron requested a review from llvm-beanz July 11, 2025 17:48

Warns when spec const default determines derivative group mode.

eb2dc82

llvm-beanz reviewed Jul 21, 2025

View reviewed changes

danbrown-amd and others added 2 commits August 13, 2025 21:52

Improves error handling and testing.

423617d

Merge branch 'main' into attr-arg-consts

793f8f1

		static uint32_t GetIntConstAttrArg(ASTContext &astContext, const Expr *expr,
		uint32_t defaultVal = 0) {

	}
	llvm_unreachable("Expression must be a constant expression or spec constant")
	}

Allows Vulkan spec constants as attribute arguments. #7439

Are you sure you want to change the base?

Allows Vulkan spec constants as attribute arguments. #7439

Uh oh!

Conversation

danbrown-amd commented May 8, 2025 • edited by s-perron Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

s-perron left a comment

Choose a reason for hiding this comment

Uh oh!

danbrown-amd commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-perron commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-perron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvm-beanz 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!

Uh oh!

danbrown-amd commented May 8, 2025 •

edited by s-perron

Loading

github-actions bot commented May 8, 2025 •

edited

Loading

danbrown-amd commented Jun 17, 2025 •

edited

Loading

s-perron commented Jul 7, 2025 •

edited

Loading