Skip to content

[DirectX] Fix order of v2::DescriptorRange #145555

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 5 commits into from
Jun 24, 2025
Merged

[DirectX] Fix order of v2::DescriptorRange #145555

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
24cbe8b
fix bug
Jun 24, 2025
f5949b7
make code more llvmish
Jun 24, 2025
cc55404
clean up
Jun 24, 2025
e86dc8e
address comments
Jun 24, 2025
ced4e8f
address comments
Jun 24, 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
13 changes: 11 additions & 2 deletions llvm/include/llvm/BinaryFormat/DXContainer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,10 +740,19 @@ struct RootDescriptor : public v1::RootDescriptor {
}
};

struct DescriptorRange : public v1::DescriptorRange {
struct DescriptorRange {
Copy link
Contributor

@inbelic inbelic Jun 24, 2025

Choose a reason for hiding this comment

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

Ah I see now. The old struct is still defined in the v1 namespace and this is defined in the v2 namespace.

uint32_t RangeType;
uint32_t NumDescriptors;
uint32_t BaseShaderRegister;
uint32_t RegisterSpace;
uint32_t Flags;
uint32_t OffsetInDescriptorsFromTableStart;
void swapBytes() {
v1::DescriptorRange::swapBytes();
sys::swapByteOrder(RangeType);
sys::swapByteOrder(NumDescriptors);
sys::swapByteOrder(BaseShaderRegister);
sys::swapByteOrder(RegisterSpace);
sys::swapByteOrder(OffsetInDescriptorsFromTableStart);
sys::swapByteOrder(Flags);
}
};
Expand Down
24 changes: 7 additions & 17 deletions llvm/include/llvm/Object/DXContainer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,19 +178,14 @@ struct RootDescriptorView : RootParameterView {
return readParameter<dxbc::RTS0::v2::RootDescriptor>();
}
};

struct DescriptorTable {
template <typename T> struct DescriptorTable {
uint32_t NumRanges;
uint32_t RangesOffset;
ViewArray<dxbc::RTS0::v2::DescriptorRange> Ranges;
ViewArray<T> Ranges;

typename ViewArray<dxbc::RTS0::v2::DescriptorRange>::iterator begin() const {
return Ranges.begin();
}
typename ViewArray<T>::iterator begin() const { return Ranges.begin(); }

typename ViewArray<dxbc::RTS0::v2::DescriptorRange>::iterator end() const {
return Ranges.end();
}
typename ViewArray<T>::iterator end() const { return Ranges.end(); }
};

struct DescriptorTableView : RootParameterView {
Expand All @@ -200,9 +195,9 @@ struct DescriptorTableView : RootParameterView {
}

// Define a type alias to access the template parameter from inside classof
llvm::Expected<DescriptorTable> read(uint32_t Version) {
template <typename T> llvm::Expected<DescriptorTable<T>> read() {
const char *Current = ParamData.begin();
DescriptorTable Table;
DescriptorTable<T> Table;

Table.NumRanges =
support::endian::read<uint32_t, llvm::endianness::little>(Current);
Expand All @@ -212,13 +207,8 @@ struct DescriptorTableView : RootParameterView {
support::endian::read<uint32_t, llvm::endianness::little>(Current);
Current += sizeof(uint32_t);

size_t RangeSize = sizeof(dxbc::RTS0::v1::DescriptorRange);
if (Version > 1)
RangeSize = sizeof(dxbc::RTS0::v2::DescriptorRange);

Table.Ranges.Stride = RangeSize;
Table.Ranges.Data =
ParamData.substr(2 * sizeof(uint32_t), Table.NumRanges * RangeSize);
ParamData.substr(2 * sizeof(uint32_t), Table.NumRanges * sizeof(T));
Copy link
Contributor

@bogner bogner Jun 24, 2025

Choose a reason for hiding this comment

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

Would it be clearer to use Table.NumRanges * Tabled.Ranges.Stride here?

return Table;
}
};
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/MC/DXContainerRootSignature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,10 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
llvm::endianness::little);
support::endian::write(BOS, Range.RegisterSpace,
llvm::endianness::little);
support::endian::write(BOS, Range.OffsetInDescriptorsFromTableStart,
llvm::endianness::little);
if (Version > 1)
support::endian::write(BOS, Range.Flags, llvm::endianness::little);
support::endian::write(BOS, Range.OffsetInDescriptorsFromTableStart,
llvm::endianness::little);
}
break;
}
Expand Down
81 changes: 51 additions & 30 deletions llvm/lib/ObjectYAML/DXContainerYAML.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,47 @@ DXContainerYAML::ShaderFeatureFlags::ShaderFeatureFlags(uint64_t FlagData) {
#include "llvm/BinaryFormat/DXContainerConstants.def"
}

template <typename T>
static llvm::Error
readDescriptorRanges(DXContainerYAML::RootParameterHeaderYaml &Header,
DXContainerYAML::RootSignatureYamlDesc &RootSigDesc,
object::DirectX::DescriptorTableView *DTV) {

llvm::Expected<object::DirectX::DescriptorTable<T>> TableOrErr =
DTV->read<T>();
if (Error E = TableOrErr.takeError())
return E;
auto Table = *TableOrErr;

DXContainerYAML::RootParameterLocationYaml Location(Header);
DXContainerYAML::DescriptorTableYaml &TableYaml =
RootSigDesc.Parameters.getOrInsertTable(Location);
RootSigDesc.Parameters.insertLocation(Location);

TableYaml.NumRanges = Table.NumRanges;
TableYaml.RangesOffset = Table.RangesOffset;

for (const auto &R : Table.Ranges) {
DXContainerYAML::DescriptorRangeYaml NewR;
NewR.OffsetInDescriptorsFromTableStart =
R.OffsetInDescriptorsFromTableStart;
NewR.NumDescriptors = R.NumDescriptors;
NewR.BaseShaderRegister = R.BaseShaderRegister;
NewR.RegisterSpace = R.RegisterSpace;
NewR.RangeType = R.RangeType;
if constexpr (std::is_same_v<T, dxbc::RTS0::v2::DescriptorRange>) {
// Set all flag fields for v2
#define DESCRIPTOR_RANGE_FLAG(Num, Val) \
NewR.Val = \
(R.Flags & llvm::to_underlying(dxbc::DescriptorRangeFlag::Val)) != 0;
#include "llvm/BinaryFormat/DXContainerConstants.def"
}
TableYaml.Ranges.push_back(NewR);
}

return Error::success();
}

llvm::Expected<DXContainerYAML::RootSignatureYamlDesc>
DXContainerYAML::RootSignatureYamlDesc::create(
const object::DirectX::RootSignature &Data) {
Expand Down Expand Up @@ -107,36 +148,16 @@ DXContainerYAML::RootSignatureYamlDesc::create(
}
} else if (auto *DTV =
dyn_cast<object::DirectX::DescriptorTableView>(&ParamView)) {
llvm::Expected<object::DirectX::DescriptorTable> TableOrErr =
DTV->read(Version);
if (Error E = TableOrErr.takeError())
return std::move(E);
auto Table = *TableOrErr;
RootParameterLocationYaml Location(Header);
DescriptorTableYaml &TableYaml =
RootSigDesc.Parameters.getOrInsertTable(Location);
RootSigDesc.Parameters.insertLocation(Location);

TableYaml.NumRanges = Table.NumRanges;
TableYaml.RangesOffset = Table.RangesOffset;

for (const auto &R : Table) {
DescriptorRangeYaml NewR;

NewR.OffsetInDescriptorsFromTableStart =
R.OffsetInDescriptorsFromTableStart;
NewR.NumDescriptors = R.NumDescriptors;
NewR.BaseShaderRegister = R.BaseShaderRegister;
NewR.RegisterSpace = R.RegisterSpace;
NewR.RangeType = R.RangeType;
if (Version > 1) {
#define DESCRIPTOR_RANGE_FLAG(Num, Val) \
NewR.Val = \
(R.Flags & llvm::to_underlying(dxbc::DescriptorRangeFlag::Val)) > 0;
#include "llvm/BinaryFormat/DXContainerConstants.def"
}
TableYaml.Ranges.push_back(NewR);
}
if (Version == 1) {
if (Error E = readDescriptorRanges<dxbc::RTS0::v1::DescriptorRange>(
Header, RootSigDesc, DTV))
return std::move(E);
} else if (Version == 2) {
if (Error E = readDescriptorRanges<dxbc::RTS0::v2::DescriptorRange>(
Header, RootSigDesc, DTV))
return std::move(E);
} else
llvm_unreachable("Unknown version for DescriptorRanges");
Copy link
Contributor

@inbelic inbelic Jun 24, 2025

Choose a reason for hiding this comment

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

Not a change, but maybe such cases will be a nice reason to use the explicitly defined root signature enum when it is merged in.

}
}

Expand Down
8 changes: 4 additions & 4 deletions llvm/unittests/Object/DXContainerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1062,8 +1062,8 @@ TEST(RootSignature, ParseDescriptorTable) {
0x3c, 0x00, 0x00, 0x00, 0x11, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x03, 0x00, 0x00, 0x00, 0x24, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
0x2c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff,
0x2a, 0x00, 0x00, 0x00, 0x2b, 0x00, 0x00, 0x00, 0x29, 0x00, 0x00, 0x00,
0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x2a, 0x00, 0x00, 0x00, 0x2b, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00,
0x29, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00};
DXContainer C =
Expand All @@ -1088,7 +1088,7 @@ TEST(RootSignature, ParseDescriptorTable) {
auto *DescriptorTableView =
dyn_cast<DirectX::DescriptorTableView>(&*ParamView);
ASSERT_TRUE(DescriptorTableView != nullptr);
auto Table = DescriptorTableView->read(2);
auto Table = DescriptorTableView->read<dxbc::RTS0::v2::DescriptorRange>();

ASSERT_THAT_ERROR(Table.takeError(), Succeeded());

Expand Down Expand Up @@ -1140,7 +1140,7 @@ TEST(RootSignature, ParseDescriptorTable) {
auto *DescriptorTableView =
dyn_cast<DirectX::DescriptorTableView>(&*ParamView);
ASSERT_TRUE(DescriptorTableView != nullptr);
auto Table = DescriptorTableView->read(1);
auto Table = DescriptorTableView->read<dxbc::RTS0::v1::DescriptorRange>();

ASSERT_THAT_ERROR(Table.takeError(), Succeeded());

Expand Down
4 changes: 2 additions & 2 deletions llvm/unittests/ObjectYAML/DXContainerYAMLTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,8 +459,8 @@ TEST(RootSignature, ParseDescriptorTableV11) {
0x3c, 0x00, 0x00, 0x00, 0x11, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x03, 0x00, 0x00, 0x00, 0x24, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
0x2c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff,
0x2a, 0x00, 0x00, 0x00, 0x2b, 0x00, 0x00, 0x00, 0x29, 0x00, 0x00, 0x00,
0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x2a, 0x00, 0x00, 0x00, 0x2b, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00,
0x29, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00};

Expand Down
Loading