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 3 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
21 changes: 7 additions & 14 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,9 +207,7 @@ 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);
size_t RangeSize = sizeof(T);

Table.Ranges.Stride = RangeSize;
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.

This shouldn't be necessary any more - I believe ViewArray sets the Stride correctly

Table.Ranges.Data =
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
77 changes: 49 additions & 28 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,39 @@ DXContainerYAML::ShaderFeatureFlags::ShaderFeatureFlags(uint64_t FlagData) {
#include "llvm/BinaryFormat/DXContainerConstants.def"
}

template <typename T>
static void
addDescriptorRanges(DXContainerYAML::RootParameterHeaderYaml &Header,
DXContainerYAML::RootSignatureYamlDesc &RootSigDesc,
const object::DirectX::DescriptorTable<T> &Table) {

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);
}
}

llvm::Expected<DXContainerYAML::RootSignatureYamlDesc>
DXContainerYAML::RootSignatureYamlDesc::create(
const object::DirectX::RootSignature &Data) {
Expand Down Expand Up @@ -107,35 +140,23 @@ 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) {
llvm::Expected<
object::DirectX::DescriptorTable<dxbc::RTS0::v1::DescriptorRange>>
TableOrErr = DTV->read<dxbc::RTS0::v1::DescriptorRange>();
if (Error E = TableOrErr.takeError())
return std::move(E);
auto Table = *TableOrErr;
addDescriptorRanges(Header, RootSigDesc, Table);
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.

It would simplify things a bit to move the call to read into the templated function. That is,

template <typename T>
static llvm::Error
readDescriptorRanges(DXContainerYAML::RootParameterHeaderYaml &Header,
                     DXContainerYAML::RootSignatureYamlDesc &RootSigDesc,
                     object::DirectX::DescriptorTableView *DTV) {
  llvm::Expected<object::DirectX::DescriptorTable<T>> Table = DTV->read<T>();
  if (Error E = Table.takeError())
    return std::move(E);
  // Use Table->NumRanges, Table->Ranges, etc.
  return Error::success();

} else {
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.

Might be better to explicitly not handle versions other than 1 or 2 so that if we add versions in the future and forget to update this it's very obvious. At this point we've validated that the version is something we expect, right? In that case, we can just throw an unreachable here.

      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");

llvm::Expected<
object::DirectX::DescriptorTable<dxbc::RTS0::v2::DescriptorRange>>
TableOrErr = DTV->read<dxbc::RTS0::v2::DescriptorRange>();
if (Error E = TableOrErr.takeError())
return std::move(E);
auto Table = *TableOrErr;
addDescriptorRanges(Header, RootSigDesc, Table);
}
}
}
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