[DirectX] Adding missing descriptor table validations #153276

joaosaffran · 2025-08-12T20:22:59Z

This patch adds 2 small validation to DirectX backend. First, it checks if registers in descriptor tables are not overflowing, meaning they don't try to bind registers over the maximum allowed value, this is checked both on the offset and on the number of descriptors inside the range; second, it checks if samplers are being mixed with other resource types.
Closes: #153057, #153058

…idation/check-descriptors-are-bound

…/textures-not-bind-root-signatures

…idation/check-descriptors-are-bound

…/textures-not-bind-root-signatures

inbelic

LGTM once we handle the update function

inbelic · 2025-09-11T01:10:55Z

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

  return make_error<InvalidRSMetadataValue>("ShaderVisibility");
 }

+static uint64_t updateOngoingOffset(uint64_t CurOfset, uint64_t NumDescriptors,


nit:

Suggested change

static uint64_t updateOngoingOffset(uint64_t CurOfset, uint64_t NumDescriptors,

static uint64_t updateOngoingOffset(uint64_t CurOffset, uint64_t NumDescriptors,

Is this function required? I think we could just use the OffsetBound below in the same way as the front-end? Or we can make the function shared?

I moved this function to a common place and updated the frontend to use it. I wasn't able to get the correct behaviour by copying frontend's update function. The test rootsignature-validation-fail-appending-overflow.ll, was failing with it.
If you have something you would like me to try, please let me know.

I'm a bit concerned that the change to the frontend to use this function doesn't also include a test change - the frontend's original logic was simple saturation, and this behaviour is not the same as that. Are we missing test coverage in the frontend that would show this fixing a bug?

I think the frontend has all tests covered. I spent sometime checking for equivalent errors for the tests I've added, here are the mappings I've found:

llvm/test/CodeGen/DirectX/rootsignature-validation-fail-appending-overflow.ll = RootSignature-resource-ranges-err.hlsl#append_to_unbound_signature

llvm/test/CodeGen/DirectX/rootsignature-validation-fail-offset-overflow.ll = RootSignature-resource-ranges-err.hlsl#direct_offset_overflow_signature

llvm/test/CodeGen/DirectX/rootsignature-validation-fail-register-overflow.ll = RootSignature-err.hlsl#basic_validation_0

llvm/test/CodeGen/DirectX/rootsignature-validation-fail-sampler-mix.ll =RootSignature-resource-ranges-err.hlsl#mixed_resource_table

@inbelic, let me know if you think otherwise.

I think we should do it more like this: #153276 (comment).

To Justin's point though, I think we are missing a test-case for something like:

DescriptorTable( CBV(u0, offset = 2^32 - 3), numDescriptors = 4), // Overflows CBV(u1) // Appending to overflow )

which would only report the first overflow and not the second because the offset will be truncated to 1 when converted to a uint32_t, when using updateOngoingOffset. It should report both when using the original frontend method.

clang/lib/Sema/SemaHLSL.cpp

bogner · 2025-09-17T16:18:57Z

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

+    dxil::ResourceClass RangeType =
+        static_cast<dxil::ResourceClass>(Range.RangeType);


This cast does nothing - Range.RangeType is a dxil::ResourceClass.

bogner · 2025-09-17T16:21:22Z

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

+  if (HasSampler && HasOtherRangeType)
+    return make_error<TableSamplerMixinError>(OtherRangeType, Location);


Is it deliberate that we report the last non-sampler range type rather than the first? I think the logic would be simpler here if we just tracked the current type and errored if we ever see a sampler when we have a current type other than a sampler. Then we could just do an early return and we need fewer bookkeeping variables to keep track of. Something like (untested):

dxil::ResourceClass CurrRC = dxil::ResourceClass::Sampler; for (const mcdxbc::DescriptorRange &Range : Table.Ranges) { if (Range.RangeType == dxil::ResourceClass::Sampler && CurrRC != dxil::ResourceClass::Sampler) return make_error<TableSamplerMixinError>(CurrRC, Location); CurrRC = Range.RangeType; } return Error::success();

bogner · 2025-09-17T16:23:10Z

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

+    // Validation of NumDescriptors should have happened by this point.
+    if (Range.NumDescriptors <= 0)
+      continue;


The continue is probably fine here for the reasons you describe, but why are we checking <= 0? Since NumDescriptors is unsigned, wouldn't checking == 0 make more sense?

bogner · 2025-09-17T16:23:35Z

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

+    const dxil::ResourceClass &RangeType =
+        static_cast<dxil::ResourceClass>(Range.RangeType);


Another no-op cast

bogner · 2025-09-17T16:37:48Z

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

  return make_error<InvalidRSMetadataValue>("ShaderVisibility");
 }

+static uint64_t updateOngoingOffset(uint64_t CurOfset, uint64_t NumDescriptors,


I'm a bit concerned that the change to the frontend to use this function doesn't also include a test change - the frontend's original logic was simple saturation, and this behaviour is not the same as that. Are we missing test coverage in the frontend that would show this fixing a bug?

bogner · 2025-09-17T16:38:33Z

...test/CodeGen/DirectX/ContainerData/RootSignature-DescriptorTable-AllValidFlagCombinations.ll

-!5 = !{ !"DescriptorTable", i32 0, !6, !8, !9, !10, !11, !12, !13, !14, !15, !16, !17, !18, !19, !20 }
+!3 = !{ !5, !21 } ; list of root signature elements
+!5 = !{ !"DescriptorTable", i32 0, !10, !11, !12, !13, !14, !15, !16, !17, !18, !19, !20 }
+!21 = !{ !"DescriptorTable", i32 0,!6, !8, !9 }


Suggested change

!21 = !{ !"DescriptorTable", i32 0,!6, !8, !9 }

!21 = !{ !"DescriptorTable", i32 0, !6, !8, !9 }

bogner · 2025-09-17T16:42:20Z

llvm/include/llvm/Frontend/HLSL/RootSignatureMetadata.h

  }
 };

+class OffsetOverflowError : public ErrorInfo<OffsetOverflowError> {


Nothing needed for this PR as you're just following the established pattern, but we should probably come back and take a look at this error heirarchy at some point - I'm not really convinced we need this many distinct errors given all we ever do with them is print the error message (as opposed to handling specific errors in some way). We might be able to simplify these to something only a bit more structured than a StringError without any real loss to functionality or readability.

I've created an issue to check this in the future: #159429

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

bogner

Looks generally good. One comment about the scope of a helper function.

bogner · 2025-09-23T16:18:46Z

llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h

+LLVM_ABI uint64_t updateOngoingOffset(uint64_t CurOffset,
+                                      uint64_t NumDescriptors, uint64_t Offset);


This is only used in one place now and it's pretty specific to that place. Does it really need to be a public function? Should we just make it static in the .cpp file or inline the logic where it's used since it's only in one place?

If we are putting it here, we probably need a doc comment to explain it.

This method was originally intended to be used both in frontend and backend, but this is not needed anymore, so I've removed it in the latest commit

inbelic

I think we are accidently introducing part of the same bug that was discovered in the front end

inbelic · 2025-09-23T16:56:29Z

llvm/test/CodeGen/DirectX/rootsignature-validation-fail-appending-limits-multiples.ll

@@ -0,0 +1,16 @@
+; RUN: not opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | FileCheck %s
+; CHECK: error: Cannot append range with implicit lower bound after an unbounded range CBV(register=2, space=0).


This is technically the wrong error message and the same bug that was in the front end. Which is why we added the separate variable to track unbounded to make it easier to reason about

From offline discussion, I've updated the code to look similar to the frontend. I've also updated the error messages to make sure they are reflecting the proper error.

llvm/test/CodeGen/DirectX/rootsignature-validation-fail-register-overflow.ll

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

inbelic · 2025-09-24T16:09:26Z

llvm/test/CodeGen/DirectX/rootsignature-validation-appending-limits.ll

@@ -0,0 +1,16 @@
+; RUN: opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | FileCheck %s
+; This is correct according to DXC: https://hlsl.godbolt.org/z/KPG5o74KE


Suggested change

; This is correct according to DXC: https://hlsl.godbolt.org/z/KPG5o74KE

; A descriptor range can be placed at UINT_MAX, matching DXC's behaviour

inbelic · 2025-09-24T16:09:41Z

llvm/test/CodeGen/DirectX/rootsignature-validation-fail-appending-overflow.ll

@@ -0,0 +1,17 @@
+; RUN: not opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 | FileCheck %s
+; This test check if a resource is implicitly overflowing. That means, it is appending a resource after an unbounded range.


Suggested change

; This test check if a resource is implicitly overflowing. That means, it is appending a resource after an unbounded range.

; This test checks if a resource is implicitly overflowing. That means, it is appending a resource after an unbounded range.

This patch adds 2 small validation to DirectX backend. First, it checks if registers in descriptor tables are not overflowing, meaning they don't try to bind registers over the maximum allowed value, this is checked both on the offset and on the number of descriptors inside the range; second, it checks if samplers are being mixed with other resource types. Closes: llvm#153057, llvm#153058 --------- Co-authored-by: joaosaffran <[email protected]> Co-authored-by: Joao Saffran <{ID}+{username}@users.noreply.github.com> Co-authored-by: Joao Saffran <[email protected]>

joaosaffran added 30 commits June 26, 2025 19:34

refactoring

0e8828c

refactoring

2edd215

clean up

242545e

format

3f8dec4

formating

3b1ce3b

fix import issues

f5720af

formating

ea54904

refactoring

a49aa19

init refactoring

d90676f

adding validation

a04eb9f

clean

5994b8f

implementing

e8b14bf

finish implementing && fix tests

8f40e83

fix issue

28350b2

sync parent

4fd2e0b

sync parent

e25ee87

add validation

01a558b

address comments

881dd36

Merge branch 'refactoring/updating-return-root-sig-analysis' into val…

8779ee9

…idation/check-descriptors-are-bound

adding validation

43eb04e

fix test

c16f15b

Merge branch 'validation/check-descriptors-are-bound' into validation…

f67bec1

…/textures-not-bind-root-signatures

format

d7b4cf4

format

c7d5be7

address changes

cc5afae

fix tests

571a0ef

Merge branch 'refactoring/updating-return-root-sig-analysis' into val…

974d4bc

…idation/check-descriptors-are-bound

Merge branch 'validation/check-descriptors-are-bound' into validation…

403b546

…/textures-not-bind-root-signatures

fix test

aea75da

add preserved

e0bc862

joaosaffran requested review from inbelic and bogner September 11, 2025 00:19

joaosaffran marked this pull request as ready for review September 11, 2025 00:20

inbelic approved these changes Sep 11, 2025

View reviewed changes

joaosaffran added 3 commits September 11, 2025 12:15

making updateOngoingOffset shared

e5976ee

refactoring

eb84e14

formating

6a3c2ab

joaosaffran commented Sep 11, 2025

View reviewed changes

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

Merge branch 'main' into validation/descriptor-tables

1327b74

bogner reviewed Sep 17, 2025

View reviewed changes

addressing comments from bogner

c2d39ed

joaosaffran mentioned this pull request Sep 17, 2025

[DirectX] Simplify errors from RootSignatureMetadata.h #159429

Open

joaosaffran requested a review from bogner September 17, 2025 19:16

inbelic reviewed Sep 17, 2025

View reviewed changes

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp Outdated Show resolved Hide resolved

joaosaffran added 4 commits September 17, 2025 16:10

adding edge case test

592dc62

changing frontend check back to what it was

f3253ed

removing some copies

d442625

adding same test as frontend

f785d25

bogner approved these changes Sep 23, 2025

View reviewed changes

inbelic requested changes Sep 23, 2025

View reviewed changes

joaosaffran commented Sep 23, 2025

View reviewed changes

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp Show resolved Hide resolved

joaosaffran added 3 commits September 24, 2025 04:44

refactor to copy frontend

eefd22c

Merge branch 'main' into validation/descriptor-tables

0c508d8

fix test and change error message

b188df1

inbelic approved these changes Sep 24, 2025

View reviewed changes

joaosaffran added 2 commits September 24, 2025 10:46

format

dabb9cd

format

0e08cea

joaosaffran merged commit c06f354 into llvm:main Sep 26, 2025
10 checks passed

	static uint64_t updateOngoingOffset(uint64_t CurOfset, uint64_t NumDescriptors,
	static uint64_t updateOngoingOffset(uint64_t CurOffset, uint64_t NumDescriptors,

		dxil::ResourceClass RangeType =
		static_cast<dxil::ResourceClass>(Range.RangeType);

		if (HasSampler && HasOtherRangeType)
		return make_error<TableSamplerMixinError>(OtherRangeType, Location);

		const dxil::ResourceClass &RangeType =
		static_cast<dxil::ResourceClass>(Range.RangeType);

	!21 = !{ !"DescriptorTable", i32 0,!6, !8, !9 }
	!21 = !{ !"DescriptorTable", i32 0, !6, !8, !9 }

		LLVM_ABI uint64_t updateOngoingOffset(uint64_t CurOffset,
		uint64_t NumDescriptors, uint64_t Offset);

		@@ -0,0 +1,16 @@
		; RUN: not opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 \| FileCheck %s
		; CHECK: error: Cannot append range with implicit lower bound after an unbounded range CBV(register=2, space=0).

		@@ -0,0 +1,16 @@
		; RUN: opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 \| FileCheck %s
		; This is correct according to DXC: https://hlsl.godbolt.org/z/KPG5o74KE

	; This is correct according to DXC: https://hlsl.godbolt.org/z/KPG5o74KE
	; A descriptor range can be placed at UINT_MAX, matching DXC's behaviour

		@@ -0,0 +1,17 @@
		; RUN: not opt -S -passes='dxil-post-optimization-validation' -mtriple=dxil-pc-shadermodel6.6-compute %s 2>&1 \| FileCheck %s
		; This test check if a resource is implicitly overflowing. That means, it is appending a resource after an unbounded range.

	; This test check if a resource is implicitly overflowing. That means, it is appending a resource after an unbounded range.
	; This test checks if a resource is implicitly overflowing. That means, it is appending a resource after an unbounded range.

[DirectX] Adding missing descriptor table validations #153276

[DirectX] Adding missing descriptor table validations #153276

Uh oh!

Conversation

joaosaffran commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inbelic 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!

inbelic Sep 17, 2025 • 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!

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!

bogner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaosaffran Sep 24, 2025 • 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!

inbelic 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joaosaffran commented Aug 12, 2025 •

edited

Loading

inbelic Sep 17, 2025 •

edited

Loading

joaosaffran Sep 24, 2025 •

edited

Loading