improve error handling

Author: joaosaffran

llvm/include/llvm/BinaryFormat/DXContainer.h

@@ -550,16 +550,12 @@ static_assert(sizeof(ProgramSignatureElement) == 32,
 
 struct RootSignatureValidations {
 
-  static Expected<uint32_t> validateRootFlag(uint32_t Flags) {
-    if ((Flags & ~0x80000fff) != 0)
-      return llvm::make_error<BinaryStreamError>("Invalid flag");
-    return Flags;
+  static bool validateRootFlag(uint32_t Flags) {
+    return (Flags & ~0x80000fff) != 0;
   }
 
-  static Expected<uint32_t> validateVersion(uint32_t Version) {
-    if (Version < 1 || Version > 2)
-      return llvm::make_error<BinaryStreamError>("Invalid Version");
-    return Version;
+  static bool validateVersion(uint32_t Version) {
+    return (Version < 1 || Version > 2);
   }
 };
 

llvm/lib/Object/DXContainer.cpp

@@ -11,6 +11,7 @@
 #include "llvm/Object/Error.h"
 #include "llvm/Support/Alignment.h"
 #include "llvm/Support/Endian.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 
 using namespace llvm;
@@ -254,11 +255,9 @@ Error DirectX::RootSignature::parse(StringRef Data) {
       support::endian::read<uint32_t, llvm::endianness::little>(Current);
   Current += sizeof(uint32_t);
 
-  Expected<uint32_t> MaybeVersion =
-      dxbc::RootSignatureValidations::validateVersion(VValue);
-  if (Error E = MaybeVersion.takeError())
-    return E;
-  Version = MaybeVersion.get();
+  if (dxbc::RootSignatureValidations::validateVersion(VValue))
+    return make_error<GenericBinaryError>("Invalid Version");
+  Version = VValue;
 
   NumParameters =
       support::endian::read<uint32_t, llvm::endianness::little>(Current);
@@ -280,11 +279,9 @@ Error DirectX::RootSignature::parse(StringRef Data) {
       support::endian::read<uint32_t, llvm::endianness::little>(Current);
   Current += sizeof(uint32_t);
 
-  Expected<uint32_t> MaybeFlag =
-      dxbc::RootSignatureValidations::validateRootFlag(FValue);
-  if (Error E = MaybeFlag.takeError())
-    return E;
-  Flags = MaybeFlag.get();
+  if (dxbc::RootSignatureValidations::validateRootFlag(FValue))
+    return make_error<GenericBinaryError>("Invalid flag");
+  Flags = FValue;
 
   return Error::success();
 }

llvm/lib/Target/DirectX/DXContainerGlobals.cpp

@@ -152,16 +152,18 @@ void DXContainerGlobals::addSignature(Module &M,
 void DXContainerGlobals::addRootSignature(Module &M,
                                           SmallVector<GlobalValue *> &Globals) {
 
-  std::optional<ModuleRootSignature> MRS =
-      getAnalysis<RootSignatureAnalysisWrapper>().getRootSignature();
-  if (!MRS.has_value())
+  auto &RSA = getAnalysis<RootSignatureAnalysisWrapper>();
+
+  if (!RSA.hasRootSignature())
     return;
 
+  ModuleRootSignature MRS = RSA.getRootSignature();
+
   SmallString<256> Data;
   raw_svector_ostream OS(Data);
 
   RootSignatureHeader RSH;
-  RSH.Flags = MRS->Flags;
+  RSH.Flags = MRS.Flags;
 
   RSH.write(OS);
 

llvm/lib/Target/DirectX/DXILRootSignature.cpp

@@ -15,17 +15,23 @@
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Analysis/DXILMetadataAnalysis.h"
+#include "llvm/BinaryFormat/DXContainer.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
+#include "llvm/Support/Error.h"
+#include <optional>
 
 using namespace llvm;
 using namespace llvm::dxil;
 
-static bool reportError(Twine Message) {
-  report_fatal_error(Message, false);
+bool ModuleRootSignature::reportError(Twine Message,
+                                      DiagnosticSeverity Severity) {
+  Ctx->diagnose(DiagnosticInfoGeneric(Message, Severity));
   return true;
 }
 
@@ -130,43 +136,33 @@ bool ModuleRootSignature::parse(NamedMDNode *Root, const Function *EF) {
   return HasError;
 }
 
-bool ModuleRootSignature::validateRootFlag() {
-  // Root Element validation, as specified:
-  // https://github.com/llvm/wg-hlsl/blob/main/proposals/0002-root-signature-in-clang.md#validations-during-dxil-generation
-  if ((Flags & ~0x80000fff) != 0)
-    return reportError("Invalid flag value for RootFlag");
-
-  return false;
-}
-
 bool ModuleRootSignature::validate() {
-  if (validateRootFlag())
+  if (dxbc::RootSignatureValidations::validateRootFlag(Flags)) {
     return reportError("Invalid flag value for RootFlag");
-
+  }
   return false;
 }
 
-ModuleRootSignature ModuleRootSignature::analyzeModule(Module &M,
-                                                       const Function *F) {
-  ModuleRootSignature MRS;
+OptionalRootSignature ModuleRootSignature::analyzeModule(Module &M,
+                                                         const Function *F) {
+  ModuleRootSignature MRS(&M.getContext());
 
   NamedMDNode *RootSignatureNode = M.getNamedMetadata("dx.rootsignatures");
-  if (RootSignatureNode) {
-    if (MRS.parse(RootSignatureNode, F) || MRS.validate())
-      llvm_unreachable("Invalid Root Signature Metadata.");
-  }
+  if (RootSignatureNode == nullptr || MRS.parse(RootSignatureNode, F) ||
+      MRS.validate())
+    return std::nullopt;
 
   return MRS;
 }
 
 AnalysisKey RootSignatureAnalysis::Key;
 
-ModuleRootSignature RootSignatureAnalysis::run(Module &M,
-                                               ModuleAnalysisManager &AM) {
+OptionalRootSignature RootSignatureAnalysis::run(Module &M,
+                                                 ModuleAnalysisManager &AM) {
   auto MMI = AM.getResult<DXILMetadataAnalysis>(M);
 
   if (MMI.ShaderProfile == Triple::Library)
-    return ModuleRootSignature();
+    return std::nullopt;
 
   assert(MMI.EntryPropertyVec.size() == 1);
 
@@ -186,7 +182,6 @@ bool RootSignatureAnalysisWrapper::runOnModule(Module &M) {
 
   const Function *EntryFunction = MMI.EntryPropertyVec[0].Entry;
   MRS = ModuleRootSignature::analyzeModule(M, EntryFunction);
-
   return false;
 }
 

llvm/lib/Target/DirectX/DXILRootSignature.h

@@ -12,10 +12,13 @@
 ///
 //===----------------------------------------------------------------------===//
 
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Metadata.h"
+#include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Pass.h"
 #include <optional>
+#include <utility>
 
 namespace llvm {
 namespace dxil {
@@ -31,44 +34,52 @@ enum class RootSignatureElementKind {
 
 struct ModuleRootSignature {
   uint32_t Flags = 0;
-
-  ModuleRootSignature() = default;
-  static ModuleRootSignature analyzeModule(Module &M, const Function *F);
+  ModuleRootSignature() { Ctx = nullptr; };
+  ModuleRootSignature(LLVMContext *Ctx) : Ctx(Ctx) {}
+  static std::optional<ModuleRootSignature> analyzeModule(Module &M,
+                                                          const Function *F);
 
 private:
+  LLVMContext *Ctx;
+
   bool parse(NamedMDNode *Root, const Function *F);
   bool parseRootSignatureElement(MDNode *Element);
   bool parseRootFlags(MDNode *RootFlagNode);
 
   bool validate();
-  bool validateRootFlag();
+
+  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 = ModuleRootSignature;
+  using Result = OptionalRootSignature;
 
-  ModuleRootSignature run(Module &M, ModuleAnalysisManager &AM);
+  OptionalRootSignature run(Module &M, ModuleAnalysisManager &AM);
 };
 
 /// Wrapper pass for the legacy pass manager.
 ///
 /// This is required because the passes that will depend on this are codegen
 /// passes which run through the legacy pass manager.
 class RootSignatureAnalysisWrapper : public ModulePass {
-  std::optional<ModuleRootSignature> MRS;
+private:
+  OptionalRootSignature MRS;
 
 public:
   static char ID;
 
   RootSignatureAnalysisWrapper() : ModulePass(ID) {}
 
-  const std::optional<ModuleRootSignature> &getRootSignature() { return MRS; }
+  const ModuleRootSignature &getRootSignature() { return MRS.value(); }
+  bool hasRootSignature() { return MRS.has_value(); }
 
   bool runOnModule(Module &M) override;
 

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

@@ -2,7 +2,7 @@
 
 target triple = "dxil-unknown-shadermodel6.0-compute"
 
-; CHECK: LLVM ERROR: Invalid format for Root Signature Definition. Pairs of function, root signature expected.
+; CHECK: error: Invalid format for Root Signature Definition. Pairs of function, root signature expected.
 
 
 define void @main() #0 {

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

@@ -1,8 +1,8 @@
-; RUN: not llc %s --filetype=obj -o - 2>&1 | FileCheck %s
+; RUN: not llc %s --filetype=obj -o -
 
 target triple = "dxil-unknown-shadermodel6.0-compute"
 
-; CHECK: LLVM ERROR: Invalid Root Element: NOTRootFlags
+; expected-error@-1: Invalid Root Element: NOTRootFlags
 
 
 define void @main() #0 {

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

@@ -1,8 +1,6 @@
-; RUN: not llc %s --filetype=obj -o - 2>&1 | FileCheck %s
-
+; RUN: not llc %s --filetype=obj -o - 
 target triple = "dxil-unknown-shadermodel6.0-compute"
-
-; CHECK: LLVM ERROR: Invalid flag value for RootFlag
+; expected-error@-1: Invalid flag value for RootFlag
 
 
 define void @main() #0 {

llvm/test/CodeGen/DirectX/ContainerData/RootSignature-RootElement-Error.ll

@@ -2,7 +2,7 @@
 
 target triple = "dxil-unknown-shadermodel6.0-compute"
 
-; CHECK: LLVM ERROR: Missing Root Element Metadata Node.
+; CHECK: error: Missing Root Element Metadata Node.
 
 
 define void @main() #0 {