[DirectX] adding support in obj2yaml and yaml2obj to root constants #127840

joaosaffran · 2025-02-19T18:12:57Z

Adding support for Root Constant in MC, Object and obj2yaml and yaml2obj, this PR adds:

new structures to dxbc definition.
serialize and desirialize logic from dxcontainer to yaml
tests validating against dxc
adding support to multiple parts.

bogner

A couple of minor things but this is looking fairly close.

My one major concern is that the error path of obj2yaml behaves quite poorly and isn't tested.

Given the following:

// RUN: dxc -T cs_6_5 -rootsig-define ROOT_SIGNATURE %s -Fo %t.dxil
#define ROOT_SIGNATURE "SRV(t0),RootConstants(b0, num32BitConstants = 4),"

[numthreads(1,1,1)]
void main() {}

Running obj2yaml on the output prints "Error: Invalid value for parameter type", generates YAML like the following, and exits successfully:

  - Name:            RTS0
    Size:            72
    RootSignature:
      Version:         2
      NumStaticSamplers: 0
      StaticSamplersOffset: 72
      Parameters:
        - ParameterType:   Constants32Bit
          ShaderVisibility: All
          Constants:
            Num32BitValues:  4
            RegisterSpace:   0
            ShaderRegister:  0

Other than a single line of text that's easy to miss, there's no indication that anything is wrong here at all. We may need to think about what behaviour we want here.

bogner · 2025-04-11T00:13:28Z

llvm/include/llvm/BinaryFormat/DXContainer.h

+#define ROOT_PARAMETER(Val, Enum)                                              \
+  case Val:                                                                    \
+    return dxbc::RootParameterType::Enum;
+inline llvm::Expected<dxbc::RootParameterType>
+safeParseParameterType(uint32_t V) {
+  switch (V) {
+#include "DXContainerConstants.def"
+  }
+  return createStringError(std::errc::invalid_argument,
+                           "Invalid value for parameter type");
+}


It might be simpler to just make this a "isValid" function and let the caller figure out what they want to do with it, ie:

#define SHADER_VISIBILITY(Val, Enum) \ case Val: \ return true; inline bool isValidShaderVisibility(uint32_t V) { switch (V) { #include "DXContainerConstants.def" } return false; }

and then in DXContainerYAML:

if (!dxbc::isValidParameterType(PH.ParameterType)) { llvm::errs() << "Error: Unknown parameter: " << PH.ParameterType << "\n"; continue; } NewP.Type = static_cast<dxbc::RootParameterType>(PH.ParameterType);

bogner · 2025-04-11T00:16:45Z

llvm/lib/ObjectYAML/DXContainerYAML.cpp

+    if (Error E = ParamViewOrErr.takeError()) {
+      llvm::errs() << "Error: " << E << "\n";
+      continue;
+    }


You need to "consume" the error here, otherwise we crash like so when this is run in asserts:

Program aborted due to an unhandled Error: Invalid value for parameter type

The easiest way to do that if we just want to print it is to std::move the error into a toString call:

llvm::errs() << "Error: " << toString(std::move(E)) << "\n";

We should really have tests for the error paths here so that this is obvious.

I've added 2 new tests to address this scenario and a scenario for invalid shader visibility. I've also changed how the data is created, so we can propagate the errors properly.

llvm/unittests/Object/DXContainerTest.cpp

llvm/include/llvm/BinaryFormat/DXContainer.h

llvm/lib/Object/DXContainer.cpp

llvm/test/ObjectYAML/DXContainer/RootSignature-Flags.yaml

llvm/unittests/Object/DXContainerTest.cpp

llvm/lib/Object/DXContainer.cpp

bogner

Just nitpicks left on the code. This looks good.

Please add a couple of tests where RootParametersOffset is some number larger than 24 (one reading and one writing)

bogner · 2025-04-16T06:30:17Z

llvm/lib/ObjectYAML/DXContainerEmitter.cpp

+        switch (Param.Type) {
+
+        case static_cast<uint32_t>(dxbc::RootParameterType::Constants32Bit):


Unusual whitespace

Suggested change

switch (Param.Type) {

case static_cast<uint32_t>(dxbc::RootParameterType::Constants32Bit):

switch (Param.Type) {

case static_cast<uint32_t>(dxbc::RootParameterType::Constants32Bit):

bogner · 2025-04-16T06:36:39Z

llvm/lib/ObjectYAML/DXContainerYAML.cpp

-  Val = (Flags & (uint32_t)dxbc::RootElementFlag::Val) > 0;
+  RootSigDesc.Val = (Flags & (uint32_t)dxbc::RootElementFlag::Val) > 0;


I realize this isn't added, but it's probably clearer to use llvm::to_underlying instead of the cast here (We don't have std::to_underlying yet, that's c++23)

bogner · 2025-04-16T06:37:01Z

llvm/lib/ObjectYAML/DXContainerYAML.cpp

+  IO.mapRequired("ShaderVisibility", P.Visibility);
+
+  switch (P.Type) {
+  case static_cast<uint32_t>(dxbc::RootParameterType::Constants32Bit):


Better to use llvm::to_underlying

bogner · 2025-04-16T06:43:21Z

llvm/lib/MC/DXContainerRootSignature.cpp

+  size_t Size = sizeof(dxbc::RootSignatureHeader) +
+                Parameters.size() * sizeof(dxbc::RootParameterHeader);
+
+  for (const auto &P : Parameters) {


Best to only use auto when the type is somewhere in the expression (or it's something obvious but long like an iterator)

Suggested change

for (const auto &P : Parameters) {

for (const mcdxbc::RootParameter &P : Parameters) {

bogner · 2025-04-16T06:49:28Z

llvm/lib/MC/DXContainerRootSignature.cpp

+  support::endian::write(BOS, Flags, llvm::endianness::little);
+
+  SmallVector<uint32_t> ParamsOffsets;
+  for (const auto &P : Parameters) {


bogner · 2025-04-16T06:50:21Z

llvm/include/llvm/Object/DXContainer.h

+      DataSize = sizeof(dxbc::RootConstants);
+      break;
+    }
+    auto EndOfSectionByte = getNumStaticSamplers() == 0


size_t is clearer here.

Suggested change

auto EndOfSectionByte = getNumStaticSamplers() == 0

size_t EndOfSectionByte = getNumStaticSamplers() == 0

bogner · 2025-04-16T06:51:01Z

llvm/lib/MC/DXContainerRootSignature.cpp

+  assert(NumParameters == ParamsOffsets.size());
+  for (size_t I = 0; I < NumParameters; ++I) {
+    rewriteOffsetToCurrentByte(BOS, ParamsOffsets[I]);
+    const auto &P = Parameters[I];


Another case of auto being less clear

bogner · 2025-04-16T06:51:35Z

llvm/lib/ObjectYAML/DXContainerYAML.cpp

  uint32_t Flags = Data.getFlags();
+  for (const auto &PH : Data.param_headers()) {


This one too

bogner · 2025-04-16T17:06:44Z

Please add a couple of tests where RootParametersOffset is some number larger than 24 (one reading and one writing)

I think you misunderstood me. I didn't mean we needed tests where we had an incorrect / invalid RootParametersOffset, I meant we need tests where the RootParametersOffset is not 24, but the root signature is valid. Nothing in the root signature document suggests that this must be 24 (and why have it if it's a constant?). For example, we could have arbitrary padding after the header, or we could have the static samplers be listed before the parameters.

bogner · 2025-04-16T18:46:42Z

llvm/unittests/Object/DXContainerTest.cpp

@@ -822,14 +823,65 @@ TEST(DXCFile, MalformedSignature) {
  }
 }

+TEST(RootSignature, MalformedData) {


Need to rename this - this test isn't malformed

bogner · 2025-04-16T18:48:34Z

llvm/test/ObjectYAML/DXContainer/RootSignature-MultipleParameters.yaml

+      NumRootParameters: 2
+      RootParametersOffset: 24
+      NumStaticSamplers: 0
+      StaticSamplersOffset: 12


Why use 12 here? It's good to test that the value is round-tripped correctly, but it seems odd to use an obviously silly value (this says the samplers start in the middle of the header!). Probably better to use some number that would be after the parameters.

damyanp · 2025-04-16T21:02:39Z

llvm/lib/ObjectYAML/DXContainerEmitter.cpp

@@ -19,6 +19,7 @@
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
+#include <cstdint>


Is cstdint still needed?

Thanks, missed that. It has been removed

llvm-beanz

Two style nits, otherwise LGTM.

llvm-beanz · 2025-04-16T23:40:21Z

llvm/lib/ObjectYAML/DXContainerYAML.cpp

+    if (Error E = ParamViewOrErr.takeError()) {
+      return std::move(E);
+    }


Suggested change

if (Error E = ParamViewOrErr.takeError()) {

return std::move(E);

}

if (Error E = ParamViewOrErr.takeError())

return std::move(E);

llvm-beanz · 2025-04-16T23:40:31Z

llvm/lib/ObjectYAML/DXContainerYAML.cpp

+      if (Error E = ConstantsOrErr.takeError()) {
+        return std::move(E);
+      }


Suggested change

if (Error E = ConstantsOrErr.takeError()) {

return std::move(E);

}

if (Error E = ConstantsOrErr.takeError())

return std::move(E);

…lvm#127840) Adding support for Root Constant in MC, Object and obj2yaml and yaml2obj, this PR adds: - new structures to dxbc definition. - serialize and desirialize logic from dxcontainer to yaml - tests validating against dxc - adding support to multiple parts. Closes: llvm#126633 --------- Co-authored-by: joaosaffran <[email protected]>

joaosaffran marked this pull request as ready for review February 19, 2025 20:26

joaosaffran requested review from bogner, damyanp and llvm-beanz February 19, 2025 20:48

joaosaffran added 26 commits February 19, 2025 21:38

adding rootsignature to obj2yaml

526b34a

adding test

e84a8e1

removing old test

f25fd64

remove useless includes

16552f0

addressing comments

a6eb4c0

updating test

7d080b3

removing useless header

504527b

fix formating

2425672

renaming test

08c17bb

addressing pr comments

395a536

adding str to ROOT_ELEMENT_FLAG

f93dff9

formating

1c1edb8

refactoring to follow llvm standards

5bef775

addressing comments

628545a

clean up

c5c2ab6

remove version

b9db72c

fix pr

f4af043

adding dxil-dis test

496b922

adding compatibility test

422578f

addressing test concerns

b1423eb

clean up

b6262b6

addressing comments

16d0e8e

adding fail test

0a9e468

adding comment

8295031

adding few more tests

c8e1e38

format

434b862

joaosaffran requested a review from bogner April 10, 2025 23:59

joaosaffran assigned bogner Apr 10, 2025

bogner reviewed Apr 11, 2025

View reviewed changes

joaosaffran added 2 commits April 11, 2025 19:18

adding more tests

7343d95

adding more tests and addressing comments

6986945

joaosaffran requested a review from bogner April 11, 2025 19:46

joaosaffran commented Apr 11, 2025

View reviewed changes

llvm/unittests/Object/DXContainerTest.cpp Outdated Show resolved Hide resolved

damyanp reviewed Apr 11, 2025

View reviewed changes

adding tests and changing parameter type

c0ac522

joaosaffran requested a review from damyanp April 14, 2025 18:41

damyanp reviewed Apr 14, 2025

View reviewed changes

llvm/lib/Object/DXContainer.cpp Outdated Show resolved Hide resolved

joaosaffran requested a review from damyanp April 14, 2025 20:51

joaosaffran added 2 commits April 14, 2025 20:53

address comments

8511fa9

changing back enums to enum class

330369a

bogner reviewed Apr 16, 2025

View reviewed changes

addressing PR comments and adding more tests

d747bcc

joaosaffran requested a review from bogner April 16, 2025 16:47

adding tests and fixing code

6cef567

bogner approved these changes Apr 16, 2025

View reviewed changes

addressing comments

246d5d3

damyanp approved these changes Apr 16, 2025

View reviewed changes

clean up

bb6c0cf

llvm-beanz approved these changes Apr 16, 2025

View reviewed changes

addressing nits

7885eed

joaosaffran merged commit 53eae22 into llvm:main Apr 17, 2025
5 of 9 checks passed

damyanp moved this to Closed in HLSL Support Apr 25, 2025

joaosaffran deleted the obj2yaml/root-constants branch April 29, 2025 18:51

damyanp removed this from HLSL Support Jun 25, 2025

		switch (Param.Type) {

		case static_cast<uint32_t>(dxbc::RootParameterType::Constants32Bit):

		Val = (Flags & (uint32_t)dxbc::RootElementFlag::Val) > 0;
		RootSigDesc.Val = (Flags & (uint32_t)dxbc::RootElementFlag::Val) > 0;

	for (const auto &P : Parameters) {
	for (const mcdxbc::RootParameter &P : Parameters) {

	auto EndOfSectionByte = getNumStaticSamplers() == 0
	size_t EndOfSectionByte = getNumStaticSamplers() == 0

		uint32_t Flags = Data.getFlags();
		for (const auto &PH : Data.param_headers()) {

[DirectX] adding support in obj2yaml and yaml2obj to root constants #127840

[DirectX] adding support in obj2yaml and yaml2obj to root constants #127840

Uh oh!

Conversation

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

bogner commented Apr 16, 2025

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!

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!

Uh oh!

Uh oh!

joaosaffran commented Feb 19, 2025 •

edited

Loading