Skip to content

Commit 4fe30df

Browse files
author
joaosaffran
committed
addressing most pr comments
1 parent f0e6a46 commit 4fe30df

File tree

10 files changed

+91
-51
lines changed

10 files changed

+91
-51
lines changed

‎llvm/include/llvm/BinaryFormat/DXContainer.h

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -561,14 +561,14 @@ static_assert(sizeof(ProgramSignatureElement) == 32,
561561
"ProgramSignatureElement is misaligned");
562562

563563
struct RootConstants {
564-
uint32_t ShaderRegister;
565-
uint32_t RegisterSpace;
566-
uint32_t Num32BitValues;
564+
uint32_t Register;
565+
uint32_t Space;
566+
uint32_t NumOfConstants;
567567

568568
void swapBytes() {
569-
sys::swapByteOrder(ShaderRegister);
570-
sys::swapByteOrder(RegisterSpace);
571-
sys::swapByteOrder(Num32BitValues);
569+
sys::swapByteOrder(Register);
570+
sys::swapByteOrder(Space);
571+
sys::swapByteOrder(NumOfConstants);
572572
}
573573
};
574574

‎llvm/include/llvm/BinaryFormat/RootSignatureValidation.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,10 @@ struct RootSignatureValidations {
3030
switch (Type) {
3131
case dxbc::RootParameterType::Constants32Bit:
3232
return true;
33-
default:
33+
case RootParameterType::Empty:
3434
return false;
3535
}
36+
return false;
3637
}
3738

3839
static bool isValidShaderVisibility(dxbc::ShaderVisibility Visibility) {
@@ -47,10 +48,11 @@ struct RootSignatureValidations {
4748
case ShaderVisibility::Amplification:
4849
case ShaderVisibility::Mesh:
4950
return true;
50-
default:
51+
case ShaderVisibility::Empty:
5152
return false;
5253
}
53-
}
54+
return false;
55+
};
5456
};
5557
} // namespace dxbc
5658
} // namespace llvm

‎llvm/include/llvm/MC/DXContainerRootSignature.h

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
//===----------------------------------------------------------------------===//
88

99
#include "llvm/BinaryFormat/DXContainer.h"
10+
#include "llvm/Support/ErrorHandling.h"
1011
#include <cstdint>
1112
#include <limits>
1213

@@ -32,7 +33,20 @@ struct RootSignatureDesc {
3233
void write(raw_ostream &OS) const;
3334

3435
size_t getSize() const {
35-
return sizeof(dxbc::RootSignatureHeader) + Parameters.size_in_bytes();
36+
size_t size = sizeof(dxbc::RootSignatureHeader);
37+
38+
for (const auto &P : Parameters) {
39+
switch (P.Header.ParameterType) {
40+
41+
case dxbc::RootParameterType::Constants32Bit:
42+
size += sizeof(dxbc::RootConstants);
43+
break;
44+
case dxbc::RootParameterType::Empty:
45+
llvm_unreachable("Parameter shouldn't be Empty here");
46+
break;
47+
}
48+
}
49+
return size;
3650
}
3751
};
3852
} // namespace mcdxbc

‎llvm/include/llvm/Object/DXContainer.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ class RootSignature {
134134

135135
SmallVector<DirectX::RootParameter> Parameters;
136136

137-
using ParamsIter = SmallVector<DirectX::RootParameter>::iterator;
137+
using param_iterator = SmallVector<DirectX::RootParameter>::iterator;
138138

139139
public:
140140
RootSignature() {}
@@ -145,9 +145,11 @@ class RootSignature {
145145
uint32_t getRootParametersOffset() const { return RootParametersOffset; }
146146
uint32_t getNumStaticSamplers() const { return NumStaticSamplers; }
147147
uint32_t getStaticSamplersOffset() const { return StaticSamplersOffset; }
148-
llvm::iterator_range<const RootParameter *> getParameters() const {
148+
llvm::iterator_range<const RootParameter *> params() const {
149149
return llvm::make_range(Parameters.begin(), Parameters.end());
150150
}
151+
param_iterator param_begin() { return Parameters.begin(); }
152+
param_iterator param_end() { return Parameters.end(); }
151153
uint32_t getFlags() const { return Flags; }
152154
};
153155

‎llvm/include/llvm/ObjectYAML/DXContainerYAML.h

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,9 @@ struct ShaderHash {
7676
#define ROOT_ELEMENT_FLAG(Num, Val) bool Val = false;
7777

7878
struct RootConstantsYaml {
79-
uint32_t ShaderRegister;
80-
uint32_t RegisterSpace;
81-
uint32_t Num32BitValues;
79+
uint32_t Register;
80+
uint32_t Space;
81+
uint32_t NumOfConstants;
8282
};
8383

8484
struct RootParameterYamlDesc {
@@ -94,11 +94,11 @@ struct RootParameterYamlDesc {
9494
Offset = Parameter.Header.ParameterOffset;
9595
switch (Parameter.Header.ParameterType) {
9696

97-
case dxbc::RootParameterType::Constants32Bit: {
98-
Constants.Num32BitValues = Parameter.Constants.Num32BitValues;
99-
Constants.RegisterSpace = Parameter.Constants.RegisterSpace;
100-
Constants.ShaderRegister = Parameter.Constants.ShaderRegister;
101-
} break;
97+
case dxbc::RootParameterType::Constants32Bit:
98+
Constants.NumOfConstants = Parameter.Constants.NumOfConstants;
99+
Constants.Space = Parameter.Constants.Space;
100+
Constants.Register = Parameter.Constants.Register;
101+
break;
102102
case dxbc::RootParameterType::Empty:
103103
llvm_unreachable("Invalid Root Parameter Type. It should be verified "
104104
"before reaching here.");

‎llvm/lib/MC/DXContainerRootSignature.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ static uint32_t writePlaceholder(raw_svector_ostream &Stream) {
2121
return Offset;
2222
}
2323

24-
static void rewriteOffset(raw_svector_ostream &Stream, uint32_t Offset) {
24+
static void rewriteOffsetToCurrentByte(raw_svector_ostream &Stream,
25+
uint32_t Offset) {
2526
uint32_t Value =
2627
support::endian::byte_swap<uint32_t, llvm::endianness::little>(
2728
Stream.tell());
@@ -58,16 +59,15 @@ void RootSignatureDesc::write(raw_ostream &OS) const {
5859

5960
assert(NumParameters == ParamsOffsets.size());
6061
for (size_t I = 0; I < NumParameters; ++I) {
61-
rewriteOffset(BOS, ParamsOffsets[I]);
62+
rewriteOffsetToCurrentByte(BOS, ParamsOffsets[I]);
6263
const auto &P = Parameters[I];
6364

6465
switch (P.Header.ParameterType) {
6566
case dxbc::RootParameterType::Constants32Bit: {
66-
support::endian::write(BOS, P.Constants.ShaderRegister,
67+
support::endian::write(BOS, P.Constants.Register,
6768
llvm::endianness::little);
68-
support::endian::write(BOS, P.Constants.RegisterSpace,
69-
llvm::endianness::little);
70-
support::endian::write(BOS, P.Constants.Num32BitValues,
69+
support::endian::write(BOS, P.Constants.Space, llvm::endianness::little);
70+
support::endian::write(BOS, P.Constants.NumOfConstants,
7171
llvm::endianness::little);
7272
} break;
7373
case dxbc::RootParameterType::Empty:

‎llvm/lib/Object/DXContainer.cpp

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@
1212
#include "llvm/Object/Error.h"
1313
#include "llvm/Support/Alignment.h"
1414
#include "llvm/Support/Endian.h"
15+
#include "llvm/Support/Error.h"
1516
#include "llvm/Support/FormatVariadic.h"
17+
#include <cstdint>
1618

1719
using namespace llvm;
1820
using namespace llvm::object;
@@ -25,6 +27,18 @@ static Error validationFailed(const Twine &Msg) {
2527
return make_error<StringError>(Msg.str(), inconvertibleErrorCode());
2628
}
2729

30+
template <typename E> static Error safeConvertEnum(uint32_t Value, E &Result) {
31+
static_assert(std::is_enum_v<E>, "Template must be an enum type");
32+
33+
if (Value >= 0 && Value < static_cast<int>(E::Empty)) {
34+
Result = static_cast<E>(Value);
35+
return Error::success();
36+
}
37+
38+
// Throw an exception if the value is out of range
39+
return parseFailed("Value is not within range for enum");
40+
}
41+
2842
template <typename T>
2943
static Error readStruct(StringRef Buffer, const char *Src, T &Struct) {
3044
// Don't read before the beginning or past the end of the file
@@ -290,13 +304,17 @@ Error DirectX::RootSignature::parse(StringRef Data) {
290304
llvm::Twine(FValue));
291305
Flags = FValue;
292306

293-
Current = Begin + RootParametersOffset;
294-
for (uint32_t It = 0; It < NumParameters; It++) {
307+
assert(Current == Begin + RootParametersOffset);
308+
for (uint32_t I = 0; I < NumParameters; I++) {
295309
DirectX::RootParameter NewParam;
296310

297-
NewParam.Header.ParameterType =
298-
support::endian::read<dxbc::RootParameterType,
299-
llvm::endianness::little>(Current);
311+
uint32_t MaybeParamType =
312+
support::endian::read<uint32_t, llvm::endianness::little>(Current);
313+
314+
if (Error Err =
315+
safeConvertEnum(MaybeParamType, NewParam.Header.ParameterType))
316+
return Err;
317+
300318
if (!dxbc::RootSignatureValidations::isValidParameterType(
301319
NewParam.Header.ParameterType))
302320
return validationFailed(
@@ -305,9 +323,13 @@ Error DirectX::RootSignature::parse(StringRef Data) {
305323

306324
Current += sizeof(dxbc::RootParameterType);
307325

308-
NewParam.Header.ShaderVisibility =
309-
support::endian::read<dxbc::ShaderVisibility, llvm::endianness::little>(
310-
Current);
326+
uint32_t MaybeVisibility =
327+
support::endian::read<uint32_t, llvm::endianness::little>(Current);
328+
329+
if (Error Err =
330+
safeConvertEnum(MaybeVisibility, NewParam.Header.ShaderVisibility))
331+
return Err;
332+
311333
if (!dxbc::RootSignatureValidations::isValidShaderVisibility(
312334
NewParam.Header.ShaderVisibility))
313335
return validationFailed(

‎llvm/lib/ObjectYAML/DXContainerEmitter.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -276,11 +276,11 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
276276

277277
switch (Param.Type) {
278278

279-
case dxbc::RootParameterType::Constants32Bit: {
280-
NewParam.Constants.Num32BitValues = Param.Constants.Num32BitValues;
281-
NewParam.Constants.RegisterSpace = Param.Constants.RegisterSpace;
282-
NewParam.Constants.ShaderRegister = Param.Constants.ShaderRegister;
283-
} break;
279+
case dxbc::RootParameterType::Constants32Bit:
280+
NewParam.Constants.NumOfConstants = Param.Constants.NumOfConstants;
281+
NewParam.Constants.Space = Param.Constants.Space;
282+
NewParam.Constants.Register = Param.Constants.Register;
283+
break;
284284
case dxbc::RootParameterType::Empty:
285285
llvm_unreachable("Invalid parameter type");
286286
break;

‎llvm/lib/ObjectYAML/DXContainerYAML.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ DXContainerYAML::RootSignatureYamlDesc::RootSignatureYamlDesc(
3535
NumStaticSamplers(Data.getNumStaticSamplers()),
3636
StaticSamplersOffset(Data.getStaticSamplersOffset()) {
3737
uint32_t Flags = Data.getFlags();
38-
for (auto const &P : Data.getParameters()) {
38+
for (auto const &P : Data.params()) {
3939

4040
Parameters.push_back(RootParameterYamlDesc(P));
4141
}
@@ -224,9 +224,9 @@ void MappingTraits<DXContainerYAML::RootSignatureYamlDesc>::mapping(
224224

225225
void MappingTraits<llvm::DXContainerYAML::RootConstantsYaml>::mapping(
226226
IO &IO, llvm::DXContainerYAML::RootConstantsYaml &C) {
227-
IO.mapRequired("Num32BitValues", C.Num32BitValues);
228-
IO.mapRequired("RegisterSpace", C.RegisterSpace);
229-
IO.mapRequired("ShaderRegister", C.ShaderRegister);
227+
IO.mapRequired("Num32BitValues", C.NumOfConstants);
228+
IO.mapRequired("RegisterSpace", C.Space);
229+
IO.mapRequired("ShaderRegister", C.Register);
230230
}
231231

232232
void MappingTraits<llvm::DXContainerYAML::RootParameterYamlDesc>::mapping(

‎llvm/unittests/Object/DXContainerTest.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -829,7 +829,7 @@ TEST(RootSignature, ParseRootFlags) {
829829
0x05, 0x39, 0xE1, 0xFE, 0x31, 0x20, 0xF0, 0xC1, 0x01, 0x00, 0x00, 0x00,
830830
0x44, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x24, 0x00, 0x00, 0x00,
831831
0x52, 0x54, 0x53, 0x30, 0x18, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00,
832-
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
832+
0x00, 0x00, 0x00, 0x00, 0x18, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
833833
0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
834834
};
835835
DXContainer C =
@@ -839,7 +839,7 @@ TEST(RootSignature, ParseRootFlags) {
839839
ASSERT_TRUE(RS.has_value());
840840
ASSERT_EQ(RS->getVersion(), 2u);
841841
ASSERT_EQ(RS->getNumParameters(), 0u);
842-
ASSERT_EQ(RS->getRootParametersOffset(), 0u);
842+
ASSERT_EQ(RS->getRootParametersOffset(), 24u);
843843
ASSERT_EQ(RS->getNumStaticSamplers(), 0u);
844844
ASSERT_EQ(RS->getStaticSamplersOffset(), 0u);
845845
ASSERT_EQ(RS->getFlags(), 0x01u);
@@ -918,12 +918,12 @@ TEST(RootSignature, ParseRootConstant) {
918918
ASSERT_EQ(RS->getStaticSamplersOffset(), 44u);
919919
ASSERT_EQ(RS->getFlags(), 17u);
920920

921-
for (auto const &RootParam : RS->getParameters()) {
921+
for (auto const &RootParam : RS->params()) {
922922
ASSERT_EQ((uint32_t)RootParam.Header.ParameterType, 1u);
923923
ASSERT_EQ((uint32_t)RootParam.Header.ShaderVisibility, 2u);
924-
ASSERT_EQ(RootParam.Constants.ShaderRegister, 15u);
925-
ASSERT_EQ(RootParam.Constants.RegisterSpace, 14u);
926-
ASSERT_EQ(RootParam.Constants.Num32BitValues, 16u);
924+
ASSERT_EQ(RootParam.Constants.Register, 15u);
925+
ASSERT_EQ(RootParam.Constants.Space, 14u);
926+
ASSERT_EQ(RootParam.Constants.NumOfConstants, 16u);
927927
}
928928
}
929929
{
@@ -943,7 +943,7 @@ TEST(RootSignature, ParseRootConstant) {
943943
0x00};
944944
EXPECT_THAT_EXPECTED(
945945
DXContainer::create(getMemoryBuffer<133>(Buffer)),
946-
FailedWithMessage("unsupported parameter type value read: 255"));
946+
FailedWithMessage("Value is not within range for enum"));
947947
}
948948
{
949949
// ShaderVisibility has been set to an invalid value
@@ -962,7 +962,7 @@ TEST(RootSignature, ParseRootConstant) {
962962
0x00};
963963
EXPECT_THAT_EXPECTED(
964964
DXContainer::create(getMemoryBuffer<133>(Buffer)),
965-
FailedWithMessage("unsupported shader visility flag value read: 255"));
965+
FailedWithMessage("Value is not within range for enum"));
966966
}
967967
{
968968
// Offset has been set to an invalid value

0 commit comments

Comments
 (0)