Skip to content

[NFC] Refactoring MCDXBC to support out of order storage of root parameters #137284

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

Merged
merged 30 commits into from
May 16, 2025
Merged

[NFC] Refactoring MCDXBC to support out of order storage of root parameters #137284

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
30 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
a38f10b
refactoring mcdxbc struct to store root parameters out of order
Apr 25, 2025
9a7c359
changing name
Apr 28, 2025
d6c2b55
changing variant to host pointers
Apr 28, 2025
93e4cf2
clean up
Apr 28, 2025
b45b1b6
fix
Apr 28, 2025
f804a23
fix
Apr 28, 2025
15eb6f5
fix naming
May 5, 2025
b9d7f07
fix naming
May 5, 2025
46cc8c1
addressing comments
May 8, 2025
1b3e10a
addressing comments
May 8, 2025
1f31957
addressing comments
May 8, 2025
e8fbfce
clean up
May 8, 2025
a31e5a5
removing v parameter
May 9, 2025
a394ad0
Merge branch 'obj2yaml/root-descriptors' into refactoring/remove-union
May 9, 2025
ad415a7
clean up
May 9, 2025
8ff4845
Merge branch 'main' into refactoring/remove-union
May 9, 2025
d67f7d3
addressing comment
May 14, 2025
5453ad0
clean up
May 14, 2025
836a8a8
format
May 14, 2025
5bd57a6
adding comment
May 14, 2025
960cb9c
adrresing comments
May 14, 2025
a60c7a3
clean up
May 14, 2025
2a4c2cb
formatting
May 15, 2025
c29d3f2
addressing comments
May 15, 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
64 changes: 58 additions & 6 deletions llvm/include/llvm/MC/DXContainerRootSignature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,64 @@ namespace llvm {
class raw_ostream;
namespace mcdxbc {

struct RootParameter {
struct RootParameterInfo {
dxbc::RootParameterHeader Header;
union {
dxbc::RootConstants Constants;
dxbc::RTS0::v2::RootDescriptor Descriptor;
};
size_t Location;

RootParameterInfo() = default;

RootParameterInfo(dxbc::RootParameterHeader H, size_t L)
: Header(H), Location(L) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Clearer to call these Header and Location rather than abbreviating here

Suggested change
RootParameterInfo(dxbc::RootParameterHeader H, size_t L)
: Header(H), Location(L) {}
RootParameterInfo(dxbc::RootParameterHeader Header, size_t Location)
: Header(Header), Location(Location) {}

};

struct RootParametersContainer {
SmallVector<RootParameterInfo> ParametersInfo;

SmallVector<dxbc::RootConstants> Constants;
SmallVector<dxbc::RTS0::v2::RootDescriptor> Descriptors;

void addInfo(dxbc::RootParameterHeader H, size_t L) {
ParametersInfo.push_back(RootParameterInfo(H, L));
}

void addParameter(dxbc::RootParameterHeader H, dxbc::RootConstants C) {
addInfo(H, Constants.size());
Constants.push_back(C);
}

void addParameter(dxbc::RootParameterHeader H,
dxbc::RTS0::v2::RootDescriptor D) {
addInfo(H, Descriptors.size());
Descriptors.push_back(D);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Give all of these parameters actual names please


const std::pair<uint32_t, uint32_t>
getTypeAndLocForParameter(uint32_t Index) const {
const RootParameterInfo &Info = ParametersInfo[Index];
return {Info.Header.ParameterType, Info.Location};
}

const dxbc::RootParameterHeader &getHeader(size_t Index) const {
const RootParameterInfo &Info = ParametersInfo[Index];
return Info.Header;
}

const dxbc::RootConstants &getConstant(size_t Index) const {
return Constants[Index];
}

const dxbc::RTS0::v2::RootDescriptor &getRootDescriptor(size_t Index) const {
return Descriptors[Index];
}
Comment on lines +65 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

(I thought I commented on this already, but I think github ate it...)

Seeing these next to getTypeAndLocForParameter and getHeader, "Index" is maybe ambiguous as a variable name. Let's call these ones "Location" instead to differentiate that these are indices into the particular type of object not the index into headers used elsewhere


size_t size() const { return ParametersInfo.size(); }

SmallVector<RootParameterInfo>::const_iterator begin() const {
return ParametersInfo.begin();
}
SmallVector<RootParameterInfo>::const_iterator end() const {
return ParametersInfo.end();
}
};
struct RootSignatureDesc {

Expand All @@ -29,7 +81,7 @@ struct RootSignatureDesc {
uint32_t RootParameterOffset = 0U;
uint32_t StaticSamplersOffset = 0u;
uint32_t NumStaticSamplers = 0u;
SmallVector<mcdxbc::RootParameter> Parameters;
mcdxbc::RootParametersContainer ParametersContainer;

void write(raw_ostream &OS) const;

Expand Down
44 changes: 25 additions & 19 deletions llvm/lib/MC/DXContainerRootSignature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ static void rewriteOffsetToCurrentByte(raw_svector_ostream &Stream,

size_t RootSignatureDesc::getSize() const {
size_t Size = sizeof(dxbc::RootSignatureHeader) +
Parameters.size() * sizeof(dxbc::RootParameterHeader);
ParametersContainer.size() * sizeof(dxbc::RootParameterHeader);

for (const mcdxbc::RootParameter &P : Parameters) {
switch (P.Header.ParameterType) {
for (const auto &I : ParametersContainer) {
switch (I.Header.ParameterType) {
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
Size += sizeof(dxbc::RootConstants);
break;
Expand All @@ -48,6 +48,7 @@ size_t RootSignatureDesc::getSize() const {
break;
}
}

return Size;
}

Expand All @@ -56,7 +57,7 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
raw_svector_ostream BOS(Storage);
BOS.reserveExtraSpace(getSize());

const uint32_t NumParameters = Parameters.size();
const uint32_t NumParameters = ParametersContainer.size();

support::endian::write(BOS, Version, llvm::endianness::little);
support::endian::write(BOS, NumParameters, llvm::endianness::little);
Expand All @@ -66,7 +67,7 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
support::endian::write(BOS, Flags, llvm::endianness::little);

SmallVector<uint32_t> ParamsOffsets;
for (const mcdxbc::RootParameter &P : Parameters) {
for (const RootParameterInfo &P : ParametersContainer) {
support::endian::write(BOS, P.Header.ParameterType,
llvm::endianness::little);
support::endian::write(BOS, P.Header.ShaderVisibility,
Expand All @@ -76,29 +77,34 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
}

assert(NumParameters == ParamsOffsets.size());
for (size_t I = 0; I < NumParameters; ++I) {
const RootParameterInfo *H = ParametersContainer.begin();
Copy link
Contributor

Choose a reason for hiding this comment

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

H isn't used any more (it's merely incremented)

for (size_t I = 0; I < NumParameters; ++I, H++) {
rewriteOffsetToCurrentByte(BOS, ParamsOffsets[I]);
const mcdxbc::RootParameter &P = Parameters[I];

switch (P.Header.ParameterType) {
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
support::endian::write(BOS, P.Constants.ShaderRegister,
const auto &[Type, Loc] = ParametersContainer.getTypeAndLocForParameter(I);
switch (Type) {
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
const dxbc::RootConstants Constants =
ParametersContainer.getConstant(Loc);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be const dxbc::RootConstants &Constants - no need to copy the object here. Same for Descriptor below.

support::endian::write(BOS, Constants.ShaderRegister,
llvm::endianness::little);
support::endian::write(BOS, P.Constants.RegisterSpace,
support::endian::write(BOS, Constants.RegisterSpace,
llvm::endianness::little);
support::endian::write(BOS, P.Constants.Num32BitValues,
support::endian::write(BOS, Constants.Num32BitValues,
llvm::endianness::little);
break;
} break;
Copy link
Contributor

Choose a reason for hiding this comment

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

The break statement should be inside the block here.

Suggested change
} break;
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,
case llvm::to_underlying(dxbc::RootParameterType::UAV): {
const dxbc::RTS0::v2::RootDescriptor Descriptor =
ParametersContainer.getRootDescriptor(Loc);

support::endian::write(BOS, Descriptor.ShaderRegister,
llvm::endianness::little);
support::endian::write(BOS, P.Descriptor.RegisterSpace,
support::endian::write(BOS, Descriptor.RegisterSpace,
llvm::endianness::little);
if (Version > 1)
support::endian::write(BOS, P.Descriptor.Flags,
llvm::endianness::little);
support::endian::write(BOS, Descriptor.Flags, llvm::endianness::little);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing break;?

}
}
assert(Storage.size() == getSize());
Expand Down
36 changes: 24 additions & 12 deletions llvm/lib/ObjectYAML/DXContainerEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,27 +274,39 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
RS.StaticSamplersOffset = P.RootSignature->StaticSamplersOffset;

for (const auto &Param : P.RootSignature->Parameters) {
mcdxbc::RootParameter NewParam;
NewParam.Header = dxbc::RootParameterHeader{
Param.Type, Param.Visibility, Param.Offset};
auto Header = dxbc::RootParameterHeader{Param.Type, Param.Visibility,
Param.Offset};
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
auto Header = dxbc::RootParameterHeader{Param.Type, Param.Visibility,
Param.Offset};
dxbc::RootParameterHeader Header{Param.Type, Param.Visibility,
Param.Offset};


switch (Param.Type) {
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
NewParam.Constants.Num32BitValues = Param.Constants.Num32BitValues;
NewParam.Constants.RegisterSpace = Param.Constants.RegisterSpace;
NewParam.Constants.ShaderRegister = Param.Constants.ShaderRegister;
dxbc::RootConstants Constants;
Constants.Num32BitValues = Param.Constants.Num32BitValues;
Constants.RegisterSpace = Param.Constants.RegisterSpace;
Constants.ShaderRegister = Param.Constants.ShaderRegister;
RS.ParametersContainer.addParameter(Header, Constants);
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();
if (RS.Version == 1) {
dxbc::RTS0::v1::RootDescriptor Descriptor;
Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace;
Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister;
RS.ParametersContainer.addParameter(Header, Descriptor);
} else {
dxbc::RTS0::v2::RootDescriptor Descriptor;
Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace;
Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister;
Descriptor.Flags = Param.Descriptor.getEncodedFlags();
RS.ParametersContainer.addParameter(Header, Descriptor);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems clearer than the implicit cast to from the v1 to the v2 descriptor:

          dxbc::RTS0::v2::RootDescriptor Descriptor;
          Descriptor.RegisterSpace = Param.Descriptor.RegisterSpace;
          Descriptor.ShaderRegister = Param.Descriptor.ShaderRegister;
          if (RS.Version > 1)
            Descriptor.Flags = Param.Descriptor.getEncodedFlags();
          RS.ParametersContainer.addParameter(Header, Descriptor);

aside: We should probably make the v2::RootDescriptor constructor that takes a v1::RootDescriptor explicit - this implicit cast being legal seems dangerous

break;
default:
// Handling invalid parameter type edge case. We intentionally let
// obj2yaml/yaml2obj parse and emit invalid dxcontainer data, in order
// for that to be used as a testing tool more effectively.
RS.ParametersContainer.addInfo(Header, -1);
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 not sure, but do you think it's worth adding a addUnknownParameter or addInvalidParameter method instead of calling addInfo here directly for clarity?

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 thinks that is a good idea. It can make the code more consistent. And reduce a little of the confusion regarding this edge case.

}

RS.Parameters.push_back(NewParam);
}

RS.write(OS);
Expand Down
61 changes: 34 additions & 27 deletions llvm/lib/Target/DirectX/DXILRootSignature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,31 +75,32 @@ static bool parseRootConstants(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD,
if (RootConstantNode->getNumOperands() != 5)
return reportError(Ctx, "Invalid format for RootConstants Element");

mcdxbc::RootParameter NewParameter;
NewParameter.Header.ParameterType =
dxbc::RootParameterHeader Header;
Header.ParameterType =
llvm::to_underlying(dxbc::RootParameterType::Constants32Bit);

if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 1))
NewParameter.Header.ShaderVisibility = *Val;
Header.ShaderVisibility = *Val;
else
return reportError(Ctx, "Invalid value for ShaderVisibility");
Copy link
Contributor

Choose a reason for hiding this comment

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

Header.ParameterOffset is left uninitialized. Bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no need to initialized Header.ParameterOffset, since those will be calculated when writing, in RootSignatureDesc::write

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a bug when the structure is created here but then only partially initialized. We should probably have a comment here, or even write a zero to it with a comment that it will be overwritten later.


dxbc::RootConstants Constants;
if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 2))
NewParameter.Constants.ShaderRegister = *Val;
Constants.ShaderRegister = *Val;
else
return reportError(Ctx, "Invalid value for ShaderRegister");

if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 3))
NewParameter.Constants.RegisterSpace = *Val;
Constants.RegisterSpace = *Val;
else
return reportError(Ctx, "Invalid value for RegisterSpace");

if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 4))
NewParameter.Constants.Num32BitValues = *Val;
Constants.Num32BitValues = *Val;
else
return reportError(Ctx, "Invalid value for Num32BitValues");

RSD.Parameters.push_back(NewParameter);
RSD.ParametersContainer.addParameter(Header, Constants);

return false;
}
Expand Down Expand Up @@ -164,12 +165,12 @@ static bool validate(LLVMContext *Ctx, const mcdxbc::RootSignatureDesc &RSD) {
return reportValueError(Ctx, "RootFlags", RSD.Flags);
}

for (const mcdxbc::RootParameter &P : RSD.Parameters) {
if (!dxbc::isValidShaderVisibility(P.Header.ShaderVisibility))
for (const llvm::mcdxbc::RootParameterInfo &Info : RSD.ParametersContainer) {
if (!dxbc::isValidShaderVisibility(Info.Header.ShaderVisibility))
return reportValueError(Ctx, "ShaderVisibility",
P.Header.ShaderVisibility);
Info.Header.ShaderVisibility);

assert(dxbc::isValidParameterType(P.Header.ParameterType) &&
assert(dxbc::isValidParameterType(Info.Header.ParameterType) &&
"Invalid value for ParameterType");
}

Expand Down Expand Up @@ -287,34 +288,40 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
OS << indent(Space) << "Version: " << RS.Version << "\n";
OS << indent(Space) << "RootParametersOffset: " << RS.RootParameterOffset
<< "\n";
OS << indent(Space) << "NumParameters: " << RS.Parameters.size() << "\n";
OS << indent(Space) << "NumParameters: " << RS.ParametersContainer.size()
<< "\n";
Space++;
for (auto const &P : RS.Parameters) {
OS << indent(Space) << "- Parameter Type: " << P.Header.ParameterType
<< "\n";
for (size_t I = 0; I < RS.ParametersContainer.size(); I++) {
const auto &[Type, Loc] =
RS.ParametersContainer.getTypeAndLocForParameter(I);
const dxbc::RootParameterHeader Header =
RS.ParametersContainer.getHeader(I);

OS << indent(Space) << "- Parameter Type: " << Type << "\n";
OS << indent(Space + 2)
<< "Shader Visibility: " << P.Header.ShaderVisibility << "\n";
switch (P.Header.ParameterType) {
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit):
OS << indent(Space + 2)
<< "Register Space: " << P.Constants.RegisterSpace << "\n";
<< "Shader Visibility: " << Header.ShaderVisibility << "\n";

switch (Type) {
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): {
auto Constants = RS.ParametersContainer.getConstant(Loc);
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to spell the type out here (also we should make this a const ref instead of copying it):

Suggested change
auto Constants = RS.ParametersContainer.getConstant(Loc);
const dxbc::RootConstants &Constants =
RS.ParametersContainer.getConstant(Loc);

OS << indent(Space + 2) << "Register Space: " << Constants.RegisterSpace
<< "\n";
OS << indent(Space + 2)
<< "Shader Register: " << P.Constants.ShaderRegister << "\n";
<< "Shader Register: " << Constants.ShaderRegister << "\n";
OS << indent(Space + 2)
<< "Num 32 Bit Values: " << P.Constants.Num32BitValues << "\n";
break;
<< "Num 32 Bit Values: " << Constants.Num32BitValues << "\n";
}
}
Space--;
}
Space--;
OS << indent(Space) << "NumStaticSamplers: " << 0 << "\n";
OS << indent(Space) << "StaticSamplersOffset: " << RS.StaticSamplersOffset
<< "\n";

Space--;
// end root signature header
}

return PreservedAnalyses::all();
}
return PreservedAnalyses::all();
}

//===----------------------------------------------------------------------===//
Expand Down
Loading