Skip to content

[DXIL] Adding support to RootSignatureFlags in obj2yaml #122396

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 29 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
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
15 changes: 15 additions & 0 deletions llvm/include/llvm/BinaryFormat/DXContainer.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ struct ShaderHash {
void swapBytes() { sys::swapByteOrder(Flags); }
};

struct RootSignatureDesc {
uint32_t Size;
uint32_t Flags;

void swapBytes() {
sys::swapByteOrder(Size);
sys::swapByteOrder(Flags);
}
};

struct ContainerVersion {
uint16_t Major;
uint16_t Minor;
Expand Down Expand Up @@ -152,6 +162,11 @@ enum class FeatureFlags : uint64_t {
static_assert((uint64_t)FeatureFlags::NextUnusedBit <= 1ull << 63,
"Shader flag bits exceed enum size.");

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

PartType parsePartType(StringRef S);

struct VertexPSVInfo {
Expand Down
43 changes: 43 additions & 0 deletions llvm/include/llvm/BinaryFormat/DXContainerConstants.def
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ CONTAINER_PART(DXIL)
CONTAINER_PART(SFI0)
CONTAINER_PART(HASH)
CONTAINER_PART(PSV0)
CONTAINER_PART(RTS0)
CONTAINER_PART(ISG1)
CONTAINER_PART(OSG1)
CONTAINER_PART(PSG1)
Expand Down Expand Up @@ -52,6 +53,48 @@ SHADER_FEATURE_FLAG(31, 36, NextUnusedBit, "Next reserved shader flag bit (not a
#undef SHADER_FEATURE_FLAG
#endif // SHADER_FEATURE_FLAG

#ifdef ROOT_PARAMETER

ROOT_PARAMETER(DescriptorTable)
ROOT_PARAMETER(Constants32Bit)
ROOT_PARAMETER(CBV)
ROOT_PARAMETER(SRV)
ROOT_PARAMETER(UAV)
#undef ROOT_PARAMETER
#endif // ROOT_PARAMETER
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any uses of ROOT_PARAMETER or SHADER_VISIBILITY. Can we hold off adding them until they're used please?



#ifdef SHADER_VISIBILITY

SHADER_VISIBILITY(All)
SHADER_VISIBILITY(Vertex)
SHADER_VISIBILITY(Hull)
SHADER_VISIBILITY(Domain)
SHADER_VISIBILITY(Geometry)
SHADER_VISIBILITY(Pixel)
SHADER_VISIBILITY(Amplification)
SHADER_VISIBILITY(Mesh)
#undef SHADER_VISIBILITY
#endif // SHADER_VISIBILITY

#ifdef ROOT_ELEMENT_FLAG

ROOT_ELEMENT_FLAG(0, AllowInputAssemblerInputLayout, "The app is opting in to using the Input Assembler")
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do these descriptions of the flags show up? If they don't show up anywhere then it'd be great to remove them so I don't have to review them!

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 was added as a previous suggestion from Finn #122396 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to do everything everyone suggests (my suggestions included).

I don't understand the point of adding these descriptions if they're not used for anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, my original comment wasn't very clear then. I had intended to ask for a description of the enum as a comment, similar to the other ones. Not to add a description field for each of the flags

ROOT_ELEMENT_FLAG(1, DenyVertexShaderRootAccess, "Denies the vertex shader access to the root signature.")
ROOT_ELEMENT_FLAG(2, DenyHullShaderRootAccess, "Denies the hull shader access to the root signature.")
ROOT_ELEMENT_FLAG(3, DenyDomainShaderRootAccess, "Denies the domain shader access to the root signature.")
ROOT_ELEMENT_FLAG(4, DenyGeometryShaderRootAccess, "Denies the geometry shader access to the root signature.")
ROOT_ELEMENT_FLAG(5, DenyPixelShaderRootAccess, "Denies the pixel shader access to the root signature.")
ROOT_ELEMENT_FLAG(6, AllowStreamOutput, "The app is opting in to using Stream Output.")
ROOT_ELEMENT_FLAG(7, LocalRootSignature, "The root signature is to be used with raytracing shaders to define resource bindings sourced from shader records in shader tables.")
ROOT_ELEMENT_FLAG(8, DenyAmplificationShaderRootAccess, "Denies the amplification shader access to the root signature.")
ROOT_ELEMENT_FLAG(9, DenyMeshShaderRootAccess, "Denies the mesh shader access to the root signature.")
ROOT_ELEMENT_FLAG(10, CBVSRVUAVHeapDirectlyIndexed, "The shaders are allowed to index the CBV/SRV/UAV descriptor heap directly, using the ResourceDescriptorHeap built-in variable.")
ROOT_ELEMENT_FLAG(11, SamplerHeapDirectlyIndexed, "The shaders are allowed to index the sampler descriptor heap directly, using the SamplerDescriptorHeap built-in variable.")
#undef ROOT_ELEMENT_FLAG
#endif // ROOT_ELEMENT_FLAG


#ifdef DXIL_MODULE_FLAG

// Only save DXIL module flags which not map to feature flags here.
Expand Down
28 changes: 28 additions & 0 deletions llvm/include/llvm/MC/DXContainerRootSignature.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
//===- llvm/MC/DXContainerRootSignature.h - RootSignature -*- C++ -*- ========//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include <cstdint>
#include <limits>

namespace llvm {

class raw_ostream;

namespace mcdxbc {
struct RootSignatureHeader {
uint32_t Version = 2;
uint32_t NumParameters = 0;
uint32_t RootParametersOffset = 0;
uint32_t NumStaticSamplers = 0;
uint32_t StaticSamplersOffset = 0;
uint32_t Flags = 0;

void write(raw_ostream &OS);
};
} // namespace mcdxbc
} // namespace llvm
29 changes: 29 additions & 0 deletions llvm/include/llvm/Object/DXContainer.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,29 @@ template <typename T> struct ViewArray {
};

namespace DirectX {

class RootSignature {
private:
StringRef Data;
uint32_t Version;
uint32_t NumParameters;
uint32_t RootParametersOffset;
uint32_t NumStaticSamplers;
uint32_t StaticSamplersOffset;
uint32_t Flags;

public:
RootSignature(StringRef Data) : Data(Data) {}

Error parse();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit awkward to have the invariant that a user must call parse immediately after construction for this object to be valid. I would probably opt for making the constructor private and adding a factory method to parse a valid object, so something like:

  static Expected<RootSignature> parse(StringRef Data);

Also, do we actually need to store the reference to the string in the root signature object? Would just having the parsed integer values be sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at this a bit: this is following an existing pattern used in the same source file. I wonder if it might be better to get this change in following the same conventions used, and then do a follow up PR that just refactors all the places in here that follow that pattern. Then we can decide if it's the right thing to do or not?

Being consistent with existing code has its virtues.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds reasonable to me.

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 did that way to keep the code consistent. I like @damyanp suggestion. I can refactor all cases that this suggestion falls into in a future PR. Thoughts @bogner ?

uint32_t getVersion() const { return Version; }
uint32_t getNumParameters() const { return NumParameters; }
uint32_t getRootParametersOffset() const { return RootParametersOffset; }
uint32_t getNumStaticSamplers() const { return NumStaticSamplers; }
uint32_t getStaticSamplersOffset() const { return StaticSamplersOffset; }
uint32_t getFlags() const { return Flags; }
};

class PSVRuntimeInfo {

using ResourceArray = ViewArray<dxbc::PSV::v2::ResourceBindInfo>;
Expand Down Expand Up @@ -287,6 +310,7 @@ class DXContainer {
std::optional<uint64_t> ShaderFeatureFlags;
std::optional<dxbc::ShaderHash> Hash;
std::optional<DirectX::PSVRuntimeInfo> PSVInfo;
std::optional<DirectX::RootSignature> RootSignature;
DirectX::Signature InputSignature;
DirectX::Signature OutputSignature;
DirectX::Signature PatchConstantSignature;
Expand All @@ -296,6 +320,7 @@ class DXContainer {
Error parseDXILHeader(StringRef Part);
Error parseShaderFeatureFlags(StringRef Part);
Error parseHash(StringRef Part);
Error parseRootSignature(StringRef Part);
Error parsePSVInfo(StringRef Part);
Error parseSignature(StringRef Part, DirectX::Signature &Array);
friend class PartIterator;
Expand Down Expand Up @@ -382,6 +407,10 @@ class DXContainer {

std::optional<dxbc::ShaderHash> getShaderHash() const { return Hash; }

std::optional<DirectX::RootSignature> getRootSignature() const {
return RootSignature;
}

const std::optional<DirectX::PSVRuntimeInfo> &getPSVInfo() const {
return PSVInfo;
};
Expand Down
22 changes: 22 additions & 0 deletions llvm/include/llvm/ObjectYAML/DXContainerYAML.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include "llvm/ADT/StringRef.h"
#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/Object/DXContainer.h"
#include "llvm/ObjectYAML/YAML.h"
#include "llvm/Support/YAMLTraits.h"
#include <array>
Expand Down Expand Up @@ -72,6 +73,21 @@ struct ShaderHash {
std::vector<llvm::yaml::Hex8> Digest;
};

#define ROOT_ELEMENT_FLAG(Num, Val, Str) bool Val = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that this is copying the ShaderFeatureFlags example above, but IMO this would be much clearer if the #define was immediately before the #include. I'd be interested to know if there's a reason for this.

struct RootSignatureDesc {
RootSignatureDesc() = default;
RootSignatureDesc(const object::DirectX::RootSignature &Data);

uint32_t getEncodedFlags();
uint32_t Version;
uint32_t NumParameters;
uint32_t RootParametersOffset;
uint32_t NumStaticSamplers;
uint32_t StaticSamplersOffset;

#include "llvm/BinaryFormat/DXContainerConstants.def"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint32_t getEncodedFlags();
uint32_t Version;
uint32_t NumParameters;
uint32_t RootParametersOffset;
uint32_t NumStaticSamplers;
uint32_t StaticSamplersOffset;
#include "llvm/BinaryFormat/DXContainerConstants.def"
uint32_t Version;
uint32_t NumParameters;
uint32_t RootParametersOffset;
uint32_t NumStaticSamplers;
uint32_t StaticSamplersOffset;
uint32_t getEncodedFlags();
#include "llvm/BinaryFormat/DXContainerConstants.def"

IMO this makes it a bit clearer what's going on and less likely to be confused that there's a getEncodedFlags member variable.

(also, github has messed up the diff of this suggestion and I don't know how to fix it)

};

using ResourceFlags = dxbc::PSV::ResourceFlags;
using ResourceBindInfo = dxbc::PSV::v2::ResourceBindInfo;

Expand Down Expand Up @@ -159,6 +175,7 @@ struct Part {
std::optional<ShaderHash> Hash;
std::optional<PSVInfo> Info;
std::optional<DXContainerYAML::Signature> Signature;
std::optional<DXContainerYAML::RootSignatureDesc> RootSignature;
};

struct Object {
Expand Down Expand Up @@ -241,6 +258,11 @@ template <> struct MappingTraits<DXContainerYAML::Signature> {
static void mapping(IO &IO, llvm::DXContainerYAML::Signature &El);
};

template <> struct MappingTraits<DXContainerYAML::RootSignatureDesc> {
static void mapping(IO &IO,
DXContainerYAML::RootSignatureDesc &RootSignature);
};

} // namespace yaml

} // namespace llvm
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/MC/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
add_llvm_component_library(LLVMMC
ConstantPools.cpp
DXContainerPSVInfo.cpp
DXContainerRootSignature.cpp
ELFObjectWriter.cpp
GOFFObjectWriter.cpp
MCAsmBackend.cpp
Expand Down
23 changes: 23 additions & 0 deletions llvm/lib/MC/DXContainerRootSignature.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//===- llvm/MC/DXContainerRootSignature.cpp - RootSignature -*- C++ -*-=======//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "llvm/MC/DXContainerRootSignature.h"
#include "llvm/Support/EndianStream.h"

using namespace llvm;
using namespace llvm::mcdxbc;

void RootSignatureHeader::write(raw_ostream &OS) {

support::endian::write(OS, Version, llvm::endianness::little);
support::endian::write(OS, NumParameters, llvm::endianness::little);
support::endian::write(OS, RootParametersOffset, llvm::endianness::little);
support::endian::write(OS, NumStaticSamplers, llvm::endianness::little);
support::endian::write(OS, StaticSamplersOffset, llvm::endianness::little);
support::endian::write(OS, Flags, llvm::endianness::little);
}
42 changes: 42 additions & 0 deletions llvm/lib/Object/DXContainer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/Object/Error.h"
#include "llvm/Support/Alignment.h"
#include "llvm/Support/Endian.h"
#include "llvm/Support/FormatVariadic.h"

using namespace llvm;
Expand Down Expand Up @@ -92,6 +93,15 @@ Error DXContainer::parseHash(StringRef Part) {
return Error::success();
}

Error DXContainer::parseRootSignature(StringRef Part) {
if (RootSignature)
return parseFailed("More than one RTS0 part is present in the file");
RootSignature = DirectX::RootSignature(Part);
if (Error Err = RootSignature->parse())
return Err;
return Error::success();
}

Error DXContainer::parsePSVInfo(StringRef Part) {
if (PSVInfo)
return parseFailed("More than one PSV0 part is present in the file");
Expand Down Expand Up @@ -193,6 +203,10 @@ Error DXContainer::parsePartOffsets() {
break;
case dxbc::PartType::Unknown:
break;
case dxbc::PartType::RTS0:
if (Error Err = parseRootSignature(PartData))
return Err;
break;
}
}

Expand Down Expand Up @@ -228,6 +242,34 @@ void DXContainer::PartIterator::updateIteratorImpl(const uint32_t Offset) {
IteratorState.Offset = Offset;
}

Error DirectX::RootSignature::parse() {
const char *Current = Data.begin();

Version = support::endian::read<uint32_t, llvm::endianness::little>(Current);
Current += sizeof(uint32_t);

NumParameters =
support::endian::read<uint32_t, llvm::endianness::little>(Current);
Current += sizeof(uint32_t);

RootParametersOffset =
support::endian::read<uint32_t, llvm::endianness::little>(Current);
Current += sizeof(uint32_t);

NumStaticSamplers =
support::endian::read<uint32_t, llvm::endianness::little>(Current);
Current += sizeof(uint32_t);

StaticSamplersOffset =
support::endian::read<uint32_t, llvm::endianness::little>(Current);
Current += sizeof(uint32_t);

Flags = support::endian::read<uint32_t, llvm::endianness::little>(Current);
Current += sizeof(uint32_t);

return Error::success();
}

Error DirectX::PSVRuntimeInfo::parse(uint16_t ShaderKind) {
Triple::EnvironmentType ShaderStage = dxbc::getShaderStage(ShaderKind);

Expand Down
10 changes: 10 additions & 0 deletions llvm/lib/ObjectYAML/DXContainerEmitter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/MC/DXContainerPSVInfo.h"
#include "llvm/MC/DXContainerRootSignature.h"
#include "llvm/ObjectYAML/ObjectYAML.h"
#include "llvm/ObjectYAML/yaml2obj.h"
#include "llvm/Support/Errc.h"
Expand Down Expand Up @@ -261,6 +262,15 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
}
case dxbc::PartType::Unknown:
break; // Skip any handling for unrecognized parts.
case dxbc::PartType::RTS0:
if (!P.RootSignature.has_value())
continue;

mcdxbc::RootSignatureHeader Header;
Header.Flags = P.RootSignature->getEncodedFlags();

Header.write(OS);
break;
}
uint64_t BytesWritten = OS.tell() - DataStart;
RollingOffset += BytesWritten;
Expand Down
Loading