addressing pr comments

Author: joaosaffran

llvm/include/llvm/MC/DXContainerRootSignature.h

@@ -14,15 +14,15 @@ namespace llvm {
 class raw_ostream;
 
 namespace mcdxbc {
-struct RootSignatureHeader {
+struct RootSignatureDesc {
   uint32_t Version = 2;
   uint32_t NumParameters = 0;
   uint32_t RootParametersOffset = 0;
   uint32_t NumStaticSamplers = 0;
   uint32_t StaticSamplersOffset = 0;
   uint32_t Flags = 0;
 
-  void write(raw_ostream &OS);
+  void write(raw_ostream &OS) const;
 };
 } // namespace mcdxbc
 } // namespace llvm

llvm/include/llvm/ObjectYAML/DXContainerYAML.h

@@ -74,9 +74,9 @@ struct ShaderHash {
 };
 
 #define ROOT_ELEMENT_FLAG(Num, Val) bool Val = false;
-struct RootSignatureDesc {
-  RootSignatureDesc() = default;
-  RootSignatureDesc(const object::DirectX::RootSignature &Data);
+struct RootSignatureYamlDesc {
+  RootSignatureYamlDesc() = default;
+  RootSignatureYamlDesc(const object::DirectX::RootSignature &Data);
 
   uint32_t Version;
   uint32_t NumParameters;
@@ -176,7 +176,7 @@ struct Part {
   std::optional<ShaderHash> Hash;
   std::optional<PSVInfo> Info;
   std::optional<DXContainerYAML::Signature> Signature;
-  std::optional<DXContainerYAML::RootSignatureDesc> RootSignature;
+  std::optional<DXContainerYAML::RootSignatureYamlDesc> RootSignature;
 };
 
 struct Object {
@@ -259,9 +259,9 @@ template <> struct MappingTraits<DXContainerYAML::Signature> {
   static void mapping(IO &IO, llvm::DXContainerYAML::Signature &El);
 };
 
-template <> struct MappingTraits<DXContainerYAML::RootSignatureDesc> {
+template <> struct MappingTraits<DXContainerYAML::RootSignatureYamlDesc> {
   static void mapping(IO &IO,
-                      DXContainerYAML::RootSignatureDesc &RootSignature);
+                      DXContainerYAML::RootSignatureYamlDesc &RootSignature);
 };
 
 } // namespace yaml

llvm/lib/MC/DXContainerRootSignature.cpp

@@ -12,7 +12,7 @@
 using namespace llvm;
 using namespace llvm::mcdxbc;
 
-void RootSignatureHeader::write(raw_ostream &OS) {
+void RootSignatureDesc::write(raw_ostream &OS) const {
 
   support::endian::write(OS, Version, llvm::endianness::little);
   support::endian::write(OS, NumParameters, llvm::endianness::little);

llvm/lib/ObjectYAML/DXContainerEmitter.cpp

@@ -266,15 +266,15 @@ void DXContainerWriter::writeParts(raw_ostream &OS) {
       if (!P.RootSignature.has_value())
         continue;
 
-      mcdxbc::RootSignatureHeader Header;
-      Header.Flags = P.RootSignature->getEncodedFlags();
-      Header.Version = P.RootSignature->Version;
-      Header.NumParameters = P.RootSignature->NumParameters;
-      Header.RootParametersOffset = P.RootSignature->RootParametersOffset;
-      Header.NumStaticSamplers = P.RootSignature->NumStaticSamplers;
-      Header.StaticSamplersOffset = P.RootSignature->StaticSamplersOffset;
-
-      Header.write(OS);
+      mcdxbc::RootSignatureDesc RS;
+      RS.Flags = P.RootSignature->getEncodedFlags();
+      RS.Version = P.RootSignature->Version;
+      RS.NumParameters = P.RootSignature->NumParameters;
+      RS.RootParametersOffset = P.RootSignature->RootParametersOffset;
+      RS.NumStaticSamplers = P.RootSignature->NumStaticSamplers;
+      RS.StaticSamplersOffset = P.RootSignature->StaticSamplersOffset;
+
+      RS.write(OS);
       break;
     }
     uint64_t BytesWritten = OS.tell() - DataStart;

llvm/lib/ObjectYAML/DXContainerYAML.cpp

@@ -29,7 +29,7 @@ DXContainerYAML::ShaderFeatureFlags::ShaderFeatureFlags(uint64_t FlagData) {
 #include "llvm/BinaryFormat/DXContainerConstants.def"
 }
 
-DXContainerYAML::RootSignatureDesc::RootSignatureDesc(
+DXContainerYAML::RootSignatureYamlDesc::RootSignatureYamlDesc(
     const object::DirectX::RootSignature &Data)
     : Version(Data.getVersion()), NumParameters(Data.getNumParameters()),
       RootParametersOffset(Data.getRootParametersOffset()),
@@ -41,7 +41,7 @@ DXContainerYAML::RootSignatureDesc::RootSignatureDesc(
 #include "llvm/BinaryFormat/DXContainerConstants.def"
 }
 
-uint32_t DXContainerYAML::RootSignatureDesc::getEncodedFlags() {
+uint32_t DXContainerYAML::RootSignatureYamlDesc::getEncodedFlags() {
   uint64_t Flag = 0;
 #define ROOT_ELEMENT_FLAG(Num, Val)                                            \
   if (Val)                                                                     \
@@ -209,8 +209,8 @@ void MappingTraits<DXContainerYAML::Signature>::mapping(
   IO.mapRequired("Parameters", S.Parameters);
 }
 
-void MappingTraits<DXContainerYAML::RootSignatureDesc>::mapping(
-    IO &IO, DXContainerYAML::RootSignatureDesc &S) {
+void MappingTraits<DXContainerYAML::RootSignatureYamlDesc>::mapping(
+    IO &IO, DXContainerYAML::RootSignatureYamlDesc &S) {
   IO.mapRequired("Version", S.Version);
   IO.mapRequired("NumParameters", S.NumParameters);
   IO.mapRequired("RootParametersOffset", S.RootParametersOffset);

llvm/lib/Target/DirectX/CMakeLists.txt

@@ -34,6 +34,7 @@ add_llvm_target(DirectXCodeGen
   DXILShaderFlags.cpp
   DXILTranslateMetadata.cpp
   DXILRootSignature.cpp
+  
   LINK_COMPONENTS
   Analysis
   AsmPrinter

llvm/lib/Target/DirectX/DXContainerGlobals.cpp

@@ -24,11 +24,11 @@
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/MC/DXContainerPSVInfo.h"
-#include "llvm/MC/DXContainerRootSignature.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/TargetParser/Triple.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
+#include <optional>
 
 using namespace llvm;
 using namespace llvm::dxil;
@@ -156,26 +156,24 @@ void DXContainerGlobals::addRootSignature(Module &M,
   dxil::ModuleMetadataInfo &MMI =
       getAnalysis<DXILMetadataAnalysisWrapperPass>().getModuleMetadata();
 
-  // Root Signature in Library shaders are different,
-  // since they don't use DXContainer to share it.
+  // Root Signature in Library don't compile to DXContainer.
   if (MMI.ShaderProfile == llvm::Triple::Library)
     return;
 
   assert(MMI.EntryPropertyVec.size() == 1);
 
   auto &RSA = getAnalysis<RootSignatureAnalysisWrapper>();
-  const Function *&EntryFunction = MMI.EntryPropertyVec[0].Entry;
+  const Function *EntryFunction = MMI.EntryPropertyVec[0].Entry;
+  const std::optional<RootSignatureDesc> &MaybeRS =
+      RSA.getForFunction(EntryFunction);
 
-  if (!RSA.hasForFunction(EntryFunction))
+  if (!MaybeRS.has_value())
     return;
 
-  const ModuleRootSignature &MRS = RSA.getForFunction(EntryFunction);
+  const RootSignatureDesc &RSH = MaybeRS.value();
   SmallString<256> Data;
   raw_svector_ostream OS(Data);
 
-  RootSignatureHeader RSH;
-  RSH.Flags = MRS.Flags;
-
   RSH.write(OS);
 
   Constant *Constant =

llvm/lib/Target/DirectX/DXILRootSignature.cpp

@@ -26,6 +26,7 @@
 #include "llvm/Pass.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/raw_ostream.h"
 #include <cstdint>
 #include <optional>
 #include <utility>
@@ -39,20 +40,20 @@ static bool reportError(LLVMContext *Ctx, Twine Message,
   return true;
 }
 
-static bool parseRootFlags(LLVMContext *Ctx, ModuleRootSignature &MRS,
+static bool parseRootFlags(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD,
                            MDNode *RootFlagNode) {
 
   if (RootFlagNode->getNumOperands() != 2)
     return reportError(Ctx, "Invalid format for RootFlag Element");
 
   auto *Flag = mdconst::extract<ConstantInt>(RootFlagNode->getOperand(1));
-  MRS.Flags = Flag->getZExtValue();
+  RSD.Flags = Flag->getZExtValue();
 
   return false;
 }
 
 static bool parseRootSignatureElement(LLVMContext *Ctx,
-                                      ModuleRootSignature &MRS,
+                                      mcdxbc::RootSignatureDesc &RSD,
                                       MDNode *Element) {
   MDString *ElementText = cast<MDString>(Element->getOperand(0));
   if (ElementText == nullptr)
@@ -61,21 +62,22 @@ static bool parseRootSignatureElement(LLVMContext *Ctx,
   RootSignatureElementKind ElementKind =
       StringSwitch<RootSignatureElementKind>(ElementText->getString())
           .Case("RootFlags", RootSignatureElementKind::RootFlags)
-          .Default(RootSignatureElementKind::None);
+          .Default(RootSignatureElementKind::Error);
 
   switch (ElementKind) {
 
   case RootSignatureElementKind::RootFlags:
-    return parseRootFlags(Ctx, MRS, Element);
-  case RootSignatureElementKind::None:
+    return parseRootFlags(Ctx, RSD, Element);
+  case RootSignatureElementKind::Error:
     return reportError(Ctx, "Invalid Root Signature Element: " +
                                 ElementText->getString());
   }
 
-  llvm_unreachable("Root signature element kind not expected.");
+  llvm_unreachable("Unhandled RootSignatureElementKind enum.");
 }
 
-static bool parse(LLVMContext *Ctx, ModuleRootSignature &MRS, MDNode *Node) {
+static bool parse(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD,
+                  MDNode *Node) {
   bool HasError = false;
 
   // Loop through the Root Elements of the root signature.
@@ -84,20 +86,20 @@ static bool parse(LLVMContext *Ctx, ModuleRootSignature &MRS, MDNode *Node) {
     if (Element == nullptr)
       return reportError(Ctx, "Missing Root Element Metadata Node.");
 
-    HasError = HasError || parseRootSignatureElement(Ctx, MRS, Element);
+    HasError = HasError || parseRootSignatureElement(Ctx, RSD, Element);
   }
 
   return HasError;
 }
 
-static bool validate(LLVMContext *Ctx, const ModuleRootSignature &MRS) {
-  if (!dxbc::RootSignatureValidations::isValidRootFlag(MRS.Flags)) {
+static bool validate(LLVMContext *Ctx, const mcdxbc::RootSignatureDesc &RSD) {
+  if (!dxbc::RootSignatureValidations::isValidRootFlag(RSD.Flags)) {
     return reportError(Ctx, "Invalid Root Signature flag value");
   }
   return false;
 }
 
-static SmallDenseMap<const Function *, ModuleRootSignature>
+static SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc>
 analyzeModule(Module &M) {
 
   /** Root Signature are specified as following in the metadata:
@@ -114,11 +116,11 @@ analyzeModule(Module &M) {
 
   LLVMContext *Ctx = &M.getContext();
 
-  SmallDenseMap<const Function *, ModuleRootSignature> MRSMap;
+  SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc> RSDMap;
 
   NamedMDNode *RootSignatureNode = M.getNamedMetadata("dx.rootsignatures");
   if (RootSignatureNode == nullptr)
-    return MRSMap;
+    return RSDMap;
 
   for (const auto &RSDefNode : RootSignatureNode->operands()) {
     if (RSDefNode->getNumOperands() != 2) {
@@ -153,49 +155,50 @@ analyzeModule(Module &M) {
       reportError(Ctx, "Missing Root Element List Metadata node.");
     }
 
-    ModuleRootSignature MRS;
+    mcdxbc::RootSignatureDesc RSD;
 
-    if (parse(Ctx, MRS, RootElementListNode) || validate(Ctx, MRS)) {
-      return MRSMap;
+    if (parse(Ctx, RSD, RootElementListNode) || validate(Ctx, RSD)) {
+      return RSDMap;
     }
 
-    MRSMap.insert(std::make_pair(F, MRS));
+    RSDMap.insert(std::make_pair(F, RSD));
   }
 
-  return MRSMap;
+  return RSDMap;
 }
 
 AnalysisKey RootSignatureAnalysis::Key;
 
-SmallDenseMap<const Function *, ModuleRootSignature>
+SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc>
 RootSignatureAnalysis::run(Module &M, ModuleAnalysisManager &AM) {
   return analyzeModule(M);
 }
 
 //===----------------------------------------------------------------------===//
 
-static void printSpaces(raw_ostream &Stream, unsigned int Count) {
-  for (unsigned int I = 0; I < Count; ++I) {
-    Stream << ' ';
-  }
-}
-
 PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
                                                     ModuleAnalysisManager &AM) {
 
-  SmallDenseMap<const Function *, ModuleRootSignature> &MRSMap =
+  SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc> &RSDMap =
       AM.getResult<RootSignatureAnalysis>(M);
   OS << "Root Signature Definitions"
      << "\n";
   uint8_t Space = 0;
-  for (const auto &P : MRSMap) {
-    const auto &[Function, MRS] = P;
+  for (const auto &P : RSDMap) {
+    const auto &[Function, RSD] = P;
     OS << "Definition for '" << Function->getName() << "':\n";
 
     // start root signature header
     Space++;
-    printSpaces(OS, Space);
-    OS << "Flags: " << format_hex(MRS.Flags, 8) << ":\n";
+    OS << indent(Space) << "Flags: " << format_hex(RSD.Flags, 8) << ":\n";
+    OS << indent(Space) << "Version: " << RSD.Version << ":\n";
+    OS << indent(Space) << "NumParameters: " << RSD.NumParameters << ":\n";
+    OS << indent(Space) << "RootParametersOffset: " << RSD.RootParametersOffset
+       << ":\n";
+    OS << indent(Space) << "NumStaticSamplers: " << RSD.NumStaticSamplers
+       << ":\n";
+    OS << indent(Space) << "StaticSamplersOffset: " << RSD.StaticSamplersOffset
+       << ":\n";
     Space--;
     // end root signature header
   }
@@ -205,7 +208,7 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
 
 //===----------------------------------------------------------------------===//
 bool RootSignatureAnalysisWrapper::runOnModule(Module &M) {
-  MRS = analyzeModule(M);
+  FuncToRsMap = analyzeModule(M);
   return false;
 }