Skip to content

[DirectX] Adding support for Root Descriptors in obj2yaml/yaml2obj #137259

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

[DirectX] Adding support for Root Descriptors in obj2yaml/yaml2obj #137259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0abacfc
adding support for Root Descriptors
Apr 24, 2025
8b8c02a
clean up
Apr 24, 2025
7ac9641
addressing comments
Apr 25, 2025
c105458
formating
Apr 26, 2025
efe76aa
try fix test
Apr 26, 2025
a928e9d
addressing comments
Apr 26, 2025
15eb6f5
fix naming
May 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions llvm/include/llvm/BinaryFormat/DXContainer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,11 @@ enum class RootElementFlag : uint32_t {
#include "DXContainerConstants.def"
};

#define ROOT_DESCRIPTOR_FLAG(Num, Val) Val = 1ull << Num,
enum class RootDescriptorFlag : uint32_t {
#include "DXContainerConstants.def"
};

#define ROOT_PARAMETER(Val, Enum) Enum = Val,
enum class RootParameterType : uint32_t {
#include "DXContainerConstants.def"
Expand Down Expand Up @@ -580,7 +585,28 @@ struct ProgramSignatureElement {

static_assert(sizeof(ProgramSignatureElement) == 32,
"ProgramSignatureElement is misaligned");
namespace RST0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we can either remove this name space, or, we should have all the defined structs within it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I will do it in a follow-up PR. I want to avoid noise for reviewers

namespace v0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the v0 namespace corresponds to Version 1, and the v1 namespace is Version 2?

Any reason not to use the same numbering scheme for the versions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems worthwhile to make the naming consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will address this in a follow-up PR. Existing test would also require updates, and I don't want to overpopulate this PR with such changes. Are you okay with this approach @damyanp @spall ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to get this addressed, but I'm fine with it in a different PR.

Copy link
Contributor Author

@joaosaffran joaosaffran May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@damyanp @spall @inbelic @bogner I remember one detail, DXC uses the versions as 1 and 2: https://github.com/microsoft/DirectXShaderCompiler/blob/1198c30f05ed944873ca55e89970fae407e2aacc/include/dxc/DxilRootSignature/DxilRootSignature.h#L91C1-L92C1. The version is part of the binary container, and it will lead to the parsing of several fields, I am not sure if we should change that here, since I would still need to read and write 1 and 2 as the values identifying the version, I could change those, by doing something like read(&version); version --; and write(++version); but it seems to be adding unnecessary complexity IMHO. Thoughts ?

Copy link
Contributor

@spall spall May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@damyanp @spall @inbelic @bogner I remember one detail, DXC uses the versions as 1 and 2: https://github.com/microsoft/DirectXShaderCompiler/blob/1198c30f05ed944873ca55e89970fae407e2aacc/include/dxc/DxilRootSignature/DxilRootSignature.h#L91C1-L92C1. The version is part of the binary container, and it will lead to the parsing of several fields, I am not sure if we should change that here, since I would still need to read and write 1 and 2 as the values identifying the version, I could change those, by doing something like read(&version); version --; and write(++version); but it seems to be adding unnecessary complexity IMHO. Thoughts ?

I'm not sure I understand this message, but what I was suggesting, and what I believe @damyanp was suggesting, is that in this PR you make the namespaces match the version naming convention. So v0 -> v1 and v1 -> v2.

It seems you think we were suggesting you change the Version numbers to match your namespaces?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use namespaces to distinguish these structs and not give them different names like 'RootDescriptorV1' and 'RootDescriptorV2'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is following the same pattern established in other parts of DXContainer, such as:

llvm-project/llvm/include/llvm/BinaryFormat/DXContainer.h

Line 370 in ea08dd8

struct RuntimeInfo {
.

My understating for this original decision is, newer versions of DXContainer are superset of previous versions, meaning, they add new fields, structs and flags, don't remove nor modify existing definition.

struct RootDescriptor {
uint32_t ShaderRegister;
uint32_t RegisterSpace;
void swapBytes() {
sys::swapByteOrder(ShaderRegister);
sys::swapByteOrder(RegisterSpace);
}
};
} // namespace v0

namespace v1 {
struct RootDescriptor : public v0::RootDescriptor {
uint32_t Flags;
void swapBytes() {
v0::RootDescriptor::swapBytes();
sys::swapByteOrder(Flags);
}
};
} // namespace v1
} // namespace RST0
// following dx12 naming
// https://learn.microsoft.com/en-us/windows/win32/api/d3d12/ns-d3d12-d3d12_root_constants
struct RootConstants {
Expand Down
15 changes: 15 additions & 0 deletions llvm/include/llvm/BinaryFormat/DXContainerConstants.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,24 @@ ROOT_ELEMENT_FLAG(11, SamplerHeapDirectlyIndexed)
#undef ROOT_ELEMENT_FLAG
#endif // ROOT_ELEMENT_FLAG


// ROOT_DESCRIPTOR_FLAG(bit offset for the flag, name).
#ifdef ROOT_DESCRIPTOR_FLAG

ROOT_DESCRIPTOR_FLAG(0, NONE)
ROOT_DESCRIPTOR_FLAG(1, DATA_VOLATILE)
ROOT_DESCRIPTOR_FLAG(2, DATA_STATIC_WHILE_SET_AT_EXECUTE)
ROOT_DESCRIPTOR_FLAG(3, DATA_STATIC)
#undef ROOT_DESCRIPTOR_FLAG
#endif // ROOT_DESCRIPTOR_FLAG


#ifdef ROOT_PARAMETER

ROOT_PARAMETER(1, Constants32Bit)
ROOT_PARAMETER(2, CBV)
ROOT_PARAMETER(3, SRV)
ROOT_PARAMETER(4, UAV)
#undef ROOT_PARAMETER
#endif // ROOT_PARAMETER

Expand Down
1 change: 1 addition & 0 deletions llvm/include/llvm/MC/DXContainerRootSignature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ struct RootParameter {
dxbc::RootParameterHeader Header;
union {
dxbc::RootConstants Constants;
dxbc::RST0::v1::RootDescriptor Descriptor;
};
};
struct RootSignatureDesc {
Expand Down
41 changes: 35 additions & 6 deletions llvm/include/llvm/Object/DXContainer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,16 +121,19 @@ namespace DirectX {
struct RootParameterView {
const dxbc::RootParameterHeader &Header;
StringRef ParamData;
RootParameterView(const dxbc::RootParameterHeader &H, StringRef P)
RootParameterView(uint32_t V, const dxbc::RootParameterHeader &H, StringRef P)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is V used anywhere? What does the V stand for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in RootDescriptorView, it stands for Version.

Copy link
Contributor

@damyanp damyanp May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like other constructors have V stand for "View".

But regardless, it is literally unused:

  RootParameterView(uint32_t V, const dxbc::RootParameterHeader &H, StringRef P)
      : Header(H), ParamData(P) {}

RootDescriptorView does have a Version passed to its read method. But this V passed to the RootParameterView constructor is not used.

: Header(H), ParamData(P) {}

template <typename T> Expected<T> readParameter() {
T Struct;
if (sizeof(T) != ParamData.size())
template <typename T, typename VersionT = T> Expected<T> readParameter() {
assert(sizeof(VersionT) <= sizeof(T) &&
"Parameter of higher version must inherit all previous version data "
"members");
if (sizeof(VersionT) != ParamData.size())
return make_error<GenericBinaryError>(
"Reading structure out of file bounds", object_error::parse_failed);

memcpy(&Struct, ParamData.data(), sizeof(T));
T Struct;
memcpy(&Struct, ParamData.data(), sizeof(VersionT));
Comment on lines +135 to +136
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting pretty lost trying to read this and understand what T vs VersionT is meant to mean.

But it looks like we expect VersionT to be smaller than T. Which means that this memcpy won't overwrite everything in T, which means that the rest of it is uninitialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking here: are you planning on addressing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this need addressing, the intended behaviour is to leave things uninitialized, since the fields are not being used by the current version. I am waiting on @bogner comments to see how and if this need addressing.

// DXContainer is always little endian
if (sys::IsBigEndianHost)
Struct.swapBytes();
Expand All @@ -149,6 +152,24 @@ struct RootConstantView : RootParameterView {
}
};

struct RootDescriptorView : RootParameterView {
static bool classof(const RootParameterView *V) {
return (V->Header.ParameterType ==
llvm::to_underlying(dxbc::RootParameterType::CBV) ||
V->Header.ParameterType ==
llvm::to_underlying(dxbc::RootParameterType::SRV) ||
V->Header.ParameterType ==
llvm::to_underlying(dxbc::RootParameterType::UAV));
}

llvm::Expected<dxbc::RST0::v1::RootDescriptor> read(uint32_t Version) {
if (Version == 1)
return readParameter<dxbc::RST0::v1::RootDescriptor,
dxbc::RST0::v0::RootDescriptor>();
return readParameter<dxbc::RST0::v1::RootDescriptor>();
}
};

static Error parseFailed(const Twine &Msg) {
return make_error<GenericBinaryError>(Msg.str(), object_error::parse_failed);
}
Expand Down Expand Up @@ -192,6 +213,14 @@ class RootSignature {
case dxbc::RootParameterType::Constants32Bit:
DataSize = sizeof(dxbc::RootConstants);
break;
case dxbc::RootParameterType::CBV:
case dxbc::RootParameterType::SRV:
case dxbc::RootParameterType::UAV:
if (Version == 1)
DataSize = sizeof(dxbc::RST0::v0::RootDescriptor);
else
DataSize = sizeof(dxbc::RST0::v1::RootDescriptor);
break;
}
size_t EndOfSectionByte = getNumStaticSamplers() == 0
? PartData.size()
Expand All @@ -201,7 +230,7 @@ class RootSignature {
return parseFailed("Reading structure out of file bounds");

StringRef Buff = PartData.substr(Header.ParameterOffset, DataSize);
RootParameterView View = RootParameterView(Header, Buff);
RootParameterView View = RootParameterView(Version, Header, Buff);
return View;
}
};
Expand Down
34 changes: 32 additions & 2 deletions llvm/include/llvm/ObjectYAML/DXContainerYAML.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,46 @@ struct ShaderHash {
std::vector<llvm::yaml::Hex8> Digest;
};

#define ROOT_ELEMENT_FLAG(Num, Val) bool Val = false;

struct RootConstantsYaml {
uint32_t ShaderRegister;
uint32_t RegisterSpace;
uint32_t Num32BitValues;
};

struct RootDescriptorYaml {
RootDescriptorYaml() = default;

uint32_t ShaderRegister;
uint32_t RegisterSpace;

uint32_t getEncodedFlags() const;

#define ROOT_DESCRIPTOR_FLAG(Num, Val) bool Val = false;
#include "llvm/BinaryFormat/DXContainerConstants.def"
};

struct RootParameterYamlDesc {
uint32_t Type;
uint32_t Visibility;
uint32_t Offset;
RootParameterYamlDesc(){};
RootParameterYamlDesc(uint32_t T) : Type(T) {
switch (T) {

case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
Constants = RootConstantsYaml();
break;
case llvm::to_underlying(dxbc::RootParameterType::CBV):
case llvm::to_underlying(dxbc::RootParameterType::SRV):
case llvm::to_underlying(dxbc::RootParameterType::UAV):
Descriptor = RootDescriptorYaml();
break;
}
}

union {
RootConstantsYaml Constants;
RootDescriptorYaml Descriptor;
};
};

Expand All @@ -111,6 +136,7 @@ struct RootSignatureYamlDesc {
static llvm::Expected<DXContainerYAML::RootSignatureYamlDesc>
create(const object::DirectX::RootSignature &Data);

#define ROOT_ELEMENT_FLAG(Num, Val) bool Val = false;
#include "llvm/BinaryFormat/DXContainerConstants.def"
};

Expand Down Expand Up @@ -298,6 +324,10 @@ template <> struct MappingTraits<llvm::DXContainerYAML::RootConstantsYaml> {
static void mapping(IO &IO, llvm::DXContainerYAML::RootConstantsYaml &C);
};

template <> struct MappingTraits<llvm::DXContainerYAML::RootDescriptorYaml> {
static void mapping(IO &IO, llvm::DXContainerYAML::RootDescriptorYaml &D);
};

} // namespace yaml

} // namespace llvm
Expand Down
19 changes: 19 additions & 0 deletions llvm/lib/MC/DXContainerRootSignature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@ size_t RootSignatureDesc::getSize() const {
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
Size += sizeof(dxbc::RootConstants);
break;
case llvm::to_underlying(dxbc::RootParameterType::CBV):
case llvm::to_underlying(dxbc::RootParameterType::SRV):
case llvm::to_underlying(dxbc::RootParameterType::UAV):
if (Version == 1)
Size += sizeof(dxbc::RST0::v0::RootDescriptor);
else
Size += sizeof(dxbc::RST0::v1::RootDescriptor);

break;
}
}
return Size;
Expand Down Expand Up @@ -80,6 +89,16 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
support::endian::write(BOS, P.Constants.Num32BitValues,
llvm::endianness::little);
break;
case llvm::to_underlying(dxbc::RootParameterType::CBV):
case llvm::to_underlying(dxbc::RootParameterType::SRV):
case llvm::to_underlying(dxbc::RootParameterType::UAV):
support::endian::write(BOS, P.Descriptor.ShaderRegister,
llvm::endianness::little);
support::endian::write(BOS, P.Descriptor.RegisterSpace,
llvm::endianness::little);
if (Version > 1)
support::endian::write(BOS, P.Descriptor.Flags,
llvm::endianness::little);
}
}
assert(Storage.size() == getSize());
Expand Down
8 changes: 8 additions & 0 deletions llvm/lib/ObjectYAML/DXContainerEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,14 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
NewParam.Constants.RegisterSpace = Param.Constants.RegisterSpace;
NewParam.Constants.ShaderRegister = Param.Constants.ShaderRegister;
break;
case llvm::to_underlying(dxbc::RootParameterType::SRV):
case llvm::to_underlying(dxbc::RootParameterType::UAV):
case llvm::to_underlying(dxbc::RootParameterType::CBV):
NewParam.Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace;
NewParam.Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister;
if (P.RootSignature->Version > 1)
NewParam.Descriptor.Flags = Param.Descriptor.getEncodedFlags();
break;
}

RS.Parameters.push_back(NewParam);
Expand Down
48 changes: 44 additions & 4 deletions llvm/lib/ObjectYAML/DXContainerYAML.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "llvm/ADT/STLForwardCompat.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/Object/DXContainer.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ScopedPrinter.h"
#include <cstdint>
Expand All @@ -38,8 +39,9 @@ DXContainerYAML::RootSignatureYamlDesc::create(
const object::DirectX::RootSignature &Data) {

RootSignatureYamlDesc RootSigDesc;
uint32_t Version = Data.getVersion();

RootSigDesc.Version = Data.getVersion();
RootSigDesc.Version = Version;
RootSigDesc.NumStaticSamplers = Data.getNumStaticSamplers();
RootSigDesc.StaticSamplersOffset = Data.getStaticSamplersOffset();
RootSigDesc.NumRootParameters = Data.getNumRootParameters();
Expand All @@ -48,13 +50,12 @@ DXContainerYAML::RootSignatureYamlDesc::create(
uint32_t Flags = Data.getFlags();
for (const dxbc::RootParameterHeader &PH : Data.param_headers()) {

RootParameterYamlDesc NewP;
NewP.Offset = PH.ParameterOffset;

if (!dxbc::isValidParameterType(PH.ParameterType))
return createStringError(std::errc::invalid_argument,
"Invalid value for parameter type");

RootParameterYamlDesc NewP(PH.ParameterType);
NewP.Offset = PH.ParameterOffset;
NewP.Type = PH.ParameterType;

if (!dxbc::isValidShaderVisibility(PH.ShaderVisibility))
Expand All @@ -79,7 +80,24 @@ DXContainerYAML::RootSignatureYamlDesc::create(
NewP.Constants.Num32BitValues = Constants.Num32BitValues;
NewP.Constants.ShaderRegister = Constants.ShaderRegister;
NewP.Constants.RegisterSpace = Constants.RegisterSpace;
} else if (auto *RDV =
dyn_cast<object::DirectX::RootDescriptorView>(&ParamView)) {
llvm::Expected<dxbc::RST0::v1::RootDescriptor> DescriptorOrErr =
RDV->read(Version);
if (Error E = DescriptorOrErr.takeError())
return std::move(E);
auto Descriptor = *DescriptorOrErr;
NewP.Descriptor.ShaderRegister = Descriptor.ShaderRegister;
NewP.Descriptor.RegisterSpace = Descriptor.RegisterSpace;
if (Version > 1) {
#define ROOT_DESCRIPTOR_FLAG(Num, Val) \
NewP.Descriptor.Val = \
(Descriptor.Flags & \
llvm::to_underlying(dxbc::RootDescriptorFlag::Val)) > 0;
#include "llvm/BinaryFormat/DXContainerConstants.def"
}
}

RootSigDesc.Parameters.push_back(NewP);
}
#define ROOT_ELEMENT_FLAG(Num, Val) \
Expand All @@ -89,6 +107,15 @@ DXContainerYAML::RootSignatureYamlDesc::create(
return RootSigDesc;
}

uint32_t DXContainerYAML::RootDescriptorYaml::getEncodedFlags() const {
uint64_t Flag = 0;
#define ROOT_DESCRIPTOR_FLAG(Num, Val) \
if (Val) \
Flag |= (uint32_t)dxbc::RootDescriptorFlag::Val;
#include "llvm/BinaryFormat/DXContainerConstants.def"
return Flag;
}

uint32_t DXContainerYAML::RootSignatureYamlDesc::getEncodedFlags() {
uint64_t Flag = 0;
#define ROOT_ELEMENT_FLAG(Num, Val) \
Expand Down Expand Up @@ -276,6 +303,14 @@ void MappingTraits<llvm::DXContainerYAML::RootConstantsYaml>::mapping(
IO.mapRequired("ShaderRegister", C.ShaderRegister);
}

void MappingTraits<llvm::DXContainerYAML::RootDescriptorYaml>::mapping(
IO &IO, llvm::DXContainerYAML::RootDescriptorYaml &D) {
IO.mapRequired("RegisterSpace", D.RegisterSpace);
IO.mapRequired("ShaderRegister", D.ShaderRegister);
#define ROOT_DESCRIPTOR_FLAG(Num, Val) IO.mapOptional(#Val, D.Val, false);
#include "llvm/BinaryFormat/DXContainerConstants.def"
}

void MappingTraits<llvm::DXContainerYAML::RootParameterYamlDesc>::mapping(
IO &IO, llvm::DXContainerYAML::RootParameterYamlDesc &P) {
IO.mapRequired("ParameterType", P.Type);
Expand All @@ -285,6 +320,11 @@ void MappingTraits<llvm::DXContainerYAML::RootParameterYamlDesc>::mapping(
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
IO.mapRequired("Constants", P.Constants);
break;
case llvm::to_underlying(dxbc::RootParameterType::CBV):
case llvm::to_underlying(dxbc::RootParameterType::SRV):
case llvm::to_underlying(dxbc::RootParameterType::UAV):
IO.mapRequired("Descriptor", P.Descriptor);
break;
}
}

Expand Down
Loading
Loading