addressing comments

Author: joaosaffran

llvm/include/llvm/Object/DXContainer.h

@@ -120,18 +120,20 @@ template <typename T> struct ViewArray {
 namespace DirectX {
 struct RootParameterView {
   const dxbc::RootParameterHeader &Header;
-  uint32_t Version;
   StringRef ParamData;
   RootParameterView(uint32_t V, const dxbc::RootParameterHeader &H, StringRef P)
-      : Header(H), Version(V), ParamData(P) {}
+      : Header(H), ParamData(P) {}
 
-  template <typename T> Expected<T> readParameter() {
-    T Struct;
-    if (sizeof(T) != ParamData.size())
+  template <typename T, typename VersionT = T> Expected<T> readParameter() {
+    assert(sizeof(VersionT) <= sizeof(T) &&
+           "Parameter of higher version must inherit all previous version data "
+           "members");
+    if (sizeof(VersionT) != ParamData.size())
       return make_error<GenericBinaryError>(
           "Reading structure out of file bounds", object_error::parse_failed);
 
-    memcpy(&Struct, ParamData.data(), sizeof(T));
+    T Struct;
+    memcpy(&Struct, ParamData.data(), sizeof(VersionT));
     // DXContainer is always little endian
     if (sys::IsBigEndianHost)
       Struct.swapBytes();
@@ -150,34 +152,20 @@ struct RootConstantView : RootParameterView {
   }
 };
 
-struct RootDescriptorView_V1_0 : RootParameterView {
-  static bool classof(const RootParameterView *V) {
-    return (V->Version == 1 &&
-            (V->Header.ParameterType ==
-                 llvm::to_underlying(dxbc::RootParameterType::CBV) ||
-             V->Header.ParameterType ==
-                 llvm::to_underlying(dxbc::RootParameterType::SRV) ||
-             V->Header.ParameterType ==
-                 llvm::to_underlying(dxbc::RootParameterType::UAV)));
-  }
-
-  llvm::Expected<dxbc::RST0::v0::RootDescriptor> read() {
-    return readParameter<dxbc::RST0::v0::RootDescriptor>();
-  }
-};
-
-struct RootDescriptorView_V1_1 : RootParameterView {
+struct RootDescriptorView : RootParameterView {
   static bool classof(const RootParameterView *V) {
-    return (V->Version == 2 &&
-            (V->Header.ParameterType ==
-                 llvm::to_underlying(dxbc::RootParameterType::CBV) ||
-             V->Header.ParameterType ==
-                 llvm::to_underlying(dxbc::RootParameterType::SRV) ||
-             V->Header.ParameterType ==
-                 llvm::to_underlying(dxbc::RootParameterType::UAV)));
+    return (V->Header.ParameterType ==
+                llvm::to_underlying(dxbc::RootParameterType::CBV) ||
+            V->Header.ParameterType ==
+                llvm::to_underlying(dxbc::RootParameterType::SRV) ||
+            V->Header.ParameterType ==
+                llvm::to_underlying(dxbc::RootParameterType::UAV));
   }
 
-  llvm::Expected<dxbc::RST0::v1::RootDescriptor> read() {
+  llvm::Expected<dxbc::RST0::v1::RootDescriptor> read(uint32_t Version) {
+    if (Version == 1)
+      return readParameter<dxbc::RST0::v1::RootDescriptor,
+                           dxbc::RST0::v0::RootDescriptor>();
     return readParameter<dxbc::RST0::v1::RootDescriptor>();
   }
 };

llvm/include/llvm/ObjectYAML/DXContainerYAML.h

@@ -79,7 +79,6 @@ struct RootConstantsYaml {
   uint32_t Num32BitValues;
 };
 
-#define ROOT_DESCRIPTOR_FLAG(Num, Val) bool Val = false;
 struct RootDescriptorYaml {
   RootDescriptorYaml() = default;
 
@@ -88,14 +87,15 @@ struct RootDescriptorYaml {
 
   uint32_t getEncodedFlags() const;
 
+#define ROOT_DESCRIPTOR_FLAG(Num, Val) bool Val = false;
 #include "llvm/BinaryFormat/DXContainerConstants.def"
 };
 
 struct RootParameterYamlDesc {
   uint32_t Type;
   uint32_t Visibility;
   uint32_t Offset;
-  RootParameterYamlDesc() {};
+  RootParameterYamlDesc(){};
   RootParameterYamlDesc(uint32_t T) : Type(T) {
     switch (T) {
 
@@ -116,7 +116,6 @@ struct RootParameterYamlDesc {
   };
 };
 
-#define ROOT_ELEMENT_FLAG(Num, Val) bool Val = false;
 struct RootSignatureYamlDesc {
   RootSignatureYamlDesc() = default;
 
@@ -137,6 +136,7 @@ struct RootSignatureYamlDesc {
   static llvm::Expected<DXContainerYAML::RootSignatureYamlDesc>
   create(const object::DirectX::RootSignature &Data);
 
+#define ROOT_ELEMENT_FLAG(Num, Val) bool Val = false;
 #include "llvm/BinaryFormat/DXContainerConstants.def"
 };
 

llvm/lib/ObjectYAML/DXContainerYAML.cpp

@@ -15,6 +15,7 @@
 #include "llvm/ADT/STLForwardCompat.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/BinaryFormat/DXContainer.h"
+#include "llvm/Object/DXContainer.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include <cstdint>
@@ -78,20 +79,10 @@ DXContainerYAML::RootSignatureYamlDesc::create(
       NewP.Constants.Num32BitValues = Constants.Num32BitValues;
       NewP.Constants.ShaderRegister = Constants.ShaderRegister;
       NewP.Constants.RegisterSpace = Constants.RegisterSpace;
-    } else if (auto *RDV = dyn_cast<object::DirectX::RootDescriptorView_V1_0>(
-                   &ParamView)) {
-      llvm::Expected<dxbc::RST0::v0::RootDescriptor> DescriptorOrErr =
-          RDV->read();
-      if (Error E = DescriptorOrErr.takeError())
-        return std::move(E);
-      auto Descriptor = *DescriptorOrErr;
-
-      NewP.Descriptor.ShaderRegister = Descriptor.ShaderRegister;
-      NewP.Descriptor.RegisterSpace = Descriptor.RegisterSpace;
-    } else if (auto *RDV = dyn_cast<object::DirectX::RootDescriptorView_V1_1>(
-                   &ParamView)) {
+    } else if (auto *RDV =
+                   dyn_cast<object::DirectX::RootDescriptorView>(&ParamView)) {
       llvm::Expected<dxbc::RST0::v1::RootDescriptor> DescriptorOrErr =
-          RDV->read();
+          RDV->read(Data.getVersion());
       if (Error E = DescriptorOrErr.takeError())
         return std::move(E);
       auto Descriptor = *DescriptorOrErr;

llvm/unittests/Object/DXContainerTest.cpp

@@ -994,10 +994,10 @@ TEST(RootSignature, ParseRootDescriptor) {
     auto ParamView = RS.getParameter(RootParam);
     ASSERT_THAT_ERROR(ParamView.takeError(), Succeeded());
 
-    DirectX::RootDescriptorView_V1_0 *RootDescriptorView =
-        dyn_cast<DirectX::RootDescriptorView_V1_0>(&*ParamView);
+    DirectX::RootDescriptorView *RootDescriptorView =
+        dyn_cast<DirectX::RootDescriptorView>(&*ParamView);
     ASSERT_TRUE(RootDescriptorView != nullptr);
-    auto Descriptor = RootDescriptorView->read();
+    auto Descriptor = RootDescriptorView->read(RS.getVersion());
 
     ASSERT_THAT_ERROR(Descriptor.takeError(), Succeeded());
 
@@ -1038,10 +1038,10 @@ TEST(RootSignature, ParseRootDescriptor) {
     auto ParamView = RS.getParameter(RootParam);
     ASSERT_THAT_ERROR(ParamView.takeError(), Succeeded());
 
-    DirectX::RootDescriptorView_V1_1 *RootDescriptorView =
-        dyn_cast<DirectX::RootDescriptorView_V1_1>(&*ParamView);
+    DirectX::RootDescriptorView *RootDescriptorView =
+        dyn_cast<DirectX::RootDescriptorView>(&*ParamView);
     ASSERT_TRUE(RootDescriptorView != nullptr);
-    auto Descriptor = RootDescriptorView->read();
+    auto Descriptor = RootDescriptorView->read(RS.getVersion());
 
     ASSERT_THAT_ERROR(Descriptor.takeError(), Succeeded());