addressing pr comments

Author: joaosaffran

llvm/include/llvm/BinaryFormat/DXContainer.h

@@ -548,9 +548,7 @@ static_assert(sizeof(ProgramSignatureElement) == 32,
 
 struct RootSignatureValidations {
 
-  static bool validateRootFlag(uint32_t Flags) {
-    return (Flags & ~0x80000fff) != 0;
-  }
+  static bool validateRootFlag(uint32_t Flags) { return (Flags & ~0xfff) != 0; }
 
   static bool validateVersion(uint32_t Version) {
     return !(Version == 1 || Version == 2);

llvm/lib/Target/DirectX/DXContainerGlobals.cpp

@@ -28,6 +28,7 @@
 #include "llvm/Pass.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
+#include <optional>
 
 using namespace llvm;
 using namespace llvm::dxil;
@@ -153,11 +154,12 @@ void DXContainerGlobals::addRootSignature(Module &M,
                                           SmallVector<GlobalValue *> &Globals) {
 
   auto &RSA = getAnalysis<RootSignatureAnalysisWrapper>();
+  std::optional<ModuleRootSignature> MaybeRootSignature = RSA.getResult();
 
-  if (!RSA.hasRootSignature())
+  if (!MaybeRootSignature.has_value())
     return;
 
-  ModuleRootSignature MRS = RSA.getRootSignature();
+  ModuleRootSignature MRS = MaybeRootSignature.value();
 
   SmallString<256> Data;
   raw_svector_ostream OS(Data);

llvm/lib/Target/DirectX/DXILRootSignature.cpp

@@ -29,24 +29,26 @@
 using namespace llvm;
 using namespace llvm::dxil;
 
-bool ModuleRootSignature::reportError(Twine Message,
-                                      DiagnosticSeverity Severity) {
+LLVMContext *Ctx;
+
+static bool reportError(Twine Message, DiagnosticSeverity Severity = DS_Error) {
   Ctx->diagnose(DiagnosticInfoGeneric(Message, Severity));
   return true;
 }
 
-bool ModuleRootSignature::parseRootFlags(MDNode *RootFlagNode) {
+static bool parseRootFlags(ModuleRootSignature *MRS, MDNode *RootFlagNode) {
 
   if (RootFlagNode->getNumOperands() != 2)
     return reportError("Invalid format for RootFlag Element");
 
   auto *Flag = mdconst::extract<ConstantInt>(RootFlagNode->getOperand(1));
-  this->Flags = Flag->getZExtValue();
+  MRS->Flags = Flag->getZExtValue();
 
   return false;
 }
 
-bool ModuleRootSignature::parseRootSignatureElement(MDNode *Element) {
+static bool parseRootSignatureElement(ModuleRootSignature *MRS,
+                                      MDNode *Element) {
   MDString *ElementText = cast<MDString>(Element->getOperand(0));
   if (ElementText == nullptr)
     return reportError("Invalid format for Root Element");
@@ -65,24 +67,21 @@ bool ModuleRootSignature::parseRootSignatureElement(MDNode *Element) {
 
   switch (ElementKind) {
 
-  case RootSignatureElementKind::RootFlags: {
-    return parseRootFlags(Element);
-    break;
-  }
-
+  case RootSignatureElementKind::RootFlags:
+    return parseRootFlags(MRS, Element);
   case RootSignatureElementKind::RootConstants:
   case RootSignatureElementKind::RootDescriptor:
   case RootSignatureElementKind::DescriptorTable:
   case RootSignatureElementKind::StaticSampler:
   case RootSignatureElementKind::None:
     return reportError("Invalid Root Element: " + ElementText->getString());
-    break;
   }
 
   return true;
 }
 
-bool ModuleRootSignature::parse(NamedMDNode *Root, const Function *EF) {
+static bool parse(ModuleRootSignature *MRS, NamedMDNode *Root,
+                  const Function *EF) {
   bool HasError = false;
 
   /** Root Signature are specified as following in the metadata:
@@ -93,7 +92,7 @@ bool ModuleRootSignature::parse(NamedMDNode *Root, const Function *EF) {
 
       So for each MDNode inside dx.rootsignatures NamedMDNode
       (the Root parameter of this function), the parsing process needs
-      to loop through each of it's operand and process the pairs function
+      to loop through each of its operands and process the function,
       signature pair.
    */
 
@@ -126,35 +125,36 @@ bool ModuleRootSignature::parse(NamedMDNode *Root, const Function *EF) {
       if (Element == nullptr)
         return reportError("Missing Root Element Metadata Node.");
 
-      HasError = HasError || parseRootSignatureElement(Element);
+      HasError = HasError || parseRootSignatureElement(MRS, Element);
     }
   }
   return HasError;
 }
 
-bool ModuleRootSignature::validate() {
-  if (dxbc::RootSignatureValidations::validateRootFlag(Flags)) {
+static bool validate(ModuleRootSignature *MRS) {
+  if (dxbc::RootSignatureValidations::validateRootFlag(MRS->Flags)) {
     return reportError("Invalid Root Signature flag value");
   }
   return false;
 }
 
-OptionalRootSignature ModuleRootSignature::analyzeModule(Module &M,
-                                                         const Function *F) {
-  ModuleRootSignature MRS(&M.getContext());
+std::optional<ModuleRootSignature>
+ModuleRootSignature::analyzeModule(Module &M, const Function *F) {
+  ModuleRootSignature MRS;
+  Ctx = &M.getContext();
 
   NamedMDNode *RootSignatureNode = M.getNamedMetadata("dx.rootsignatures");
-  if (RootSignatureNode == nullptr || MRS.parse(RootSignatureNode, F) ||
-      MRS.validate())
+  if (RootSignatureNode == nullptr || parse(&MRS, RootSignatureNode, F) ||
+      validate(&MRS))
     return std::nullopt;
 
   return MRS;
 }
 
 AnalysisKey RootSignatureAnalysis::Key;
 
-OptionalRootSignature RootSignatureAnalysis::run(Module &M,
-                                                 ModuleAnalysisManager &AM) {
+std::optional<ModuleRootSignature>
+RootSignatureAnalysis::run(Module &M, ModuleAnalysisManager &AM) {
   auto MMI = AM.getResult<DXILMetadataAnalysis>(M);
 
   if (MMI.ShaderProfile == Triple::Library)

llvm/lib/Target/DirectX/DXILRootSignature.h

@@ -33,36 +33,20 @@ enum class RootSignatureElementKind {
 
 struct ModuleRootSignature {
   uint32_t Flags = 0;
-  ModuleRootSignature() { Ctx = nullptr; };
   static std::optional<ModuleRootSignature> analyzeModule(Module &M,
                                                           const Function *F);
-
-private:
-  LLVMContext *Ctx;
-
-  ModuleRootSignature(LLVMContext *Ctx) : Ctx(Ctx) {}
-
-  bool parse(NamedMDNode *Root, const Function *F);
-  bool parseRootSignatureElement(MDNode *Element);
-  bool parseRootFlags(MDNode *RootFlagNode);
-
-  bool validate();
-
-  bool reportError(Twine Message, DiagnosticSeverity Severity = DS_Error);
 };
 
-using OptionalRootSignature = std::optional<ModuleRootSignature>;
-
 class RootSignatureAnalysis : public AnalysisInfoMixin<RootSignatureAnalysis> {
   friend AnalysisInfoMixin<RootSignatureAnalysis>;
   static AnalysisKey Key;
 
 public:
   RootSignatureAnalysis() = default;
 
-  using Result = OptionalRootSignature;
+  using Result = std::optional<ModuleRootSignature>;
 
-  OptionalRootSignature run(Module &M, ModuleAnalysisManager &AM);
+  std::optional<ModuleRootSignature> run(Module &M, ModuleAnalysisManager &AM);
 };
 
 /// Wrapper pass for the legacy pass manager.
@@ -71,16 +55,14 @@ class RootSignatureAnalysis : public AnalysisInfoMixin<RootSignatureAnalysis> {
 /// passes which run through the legacy pass manager.
 class RootSignatureAnalysisWrapper : public ModulePass {
 private:
-  OptionalRootSignature MRS;
+  std::optional<ModuleRootSignature> MRS;
 
 public:
   static char ID;
 
   RootSignatureAnalysisWrapper() : ModulePass(ID) {}
 
-  const ModuleRootSignature &getRootSignature() { return MRS.value(); }
-
-  bool hasRootSignature() { return MRS.has_value(); }
+  std::optional<ModuleRootSignature> getResult() const { return MRS; }
 
   bool runOnModule(Module &M) override;
 

llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Flags.ll

@@ -9,10 +9,6 @@ define void @main() #0 {
 entry:
   ret void
 }
-
-
-
-
 attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" }