addressing pr comments

Author: joaosaffran

llvm/lib/Object/DXContainer.cpp

@@ -7,10 +7,12 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Object/DXContainer.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/DXContainer.h"
 #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;
@@ -20,6 +22,10 @@ static Error parseFailed(const Twine &Msg) {
   return make_error<GenericBinaryError>(Msg.str(), object_error::parse_failed);
 }
 
+static Error validationFailed(const Twine &Msg) {
+  return make_error<StringError>(Msg.str(), inconvertibleErrorCode());
+}
+
 template <typename T>
 static Error readStruct(StringRef Buffer, const char *Src, T &Struct) {
   // Don't read before the beginning or past the end of the file
@@ -255,7 +261,8 @@ Error DirectX::RootSignature::parse(StringRef Data) {
   Current += sizeof(uint32_t);
 
   if (!dxbc::RootSignatureValidations::isValidVersion(VValue))
-    return make_error<GenericBinaryError>("Invalid Root Signature Version");
+    return validationFailed("unsupported root signature version read: " +
+                            llvm::Twine(VValue));
   Version = VValue;
 
   NumParameters =
@@ -279,7 +286,8 @@ Error DirectX::RootSignature::parse(StringRef Data) {
   Current += sizeof(uint32_t);
 
   if (!dxbc::RootSignatureValidations::isValidRootFlag(FValue))
-    return make_error<GenericBinaryError>("Invalid Root Signature flag");
+    return validationFailed("unsupported root signature flag value read: " +
+                            llvm::Twine(FValue));
   Flags = FValue;
 
   return Error::success();

llvm/lib/Target/DirectX/DXILRootSignature.cpp

@@ -1,4 +1,4 @@
-//===- DXILRootSignature.cpp - DXIL Root Signature helper objects ----===//
+//===- DXILRootSignature.cpp - DXIL Root Signature helper objects -------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -68,8 +68,8 @@ static bool parseRootSignatureElement(LLVMContext *Ctx,
   case RootSignatureElementKind::RootFlags:
     return parseRootFlags(Ctx, MRS, Element);
   case RootSignatureElementKind::None:
-    return reportError(Ctx,
-                       "Invalid Root Element: " + ElementText->getString());
+    return reportError(Ctx, "Invalid Root Signature Element: " +
+                                ElementText->getString());
   }
 
   llvm_unreachable("Root signature element kind not expected.");
@@ -78,20 +78,6 @@ static bool parseRootSignatureElement(LLVMContext *Ctx,
 static bool parse(LLVMContext *Ctx, ModuleRootSignature &MRS, MDNode *Node) {
   bool HasError = false;
 
-  /** Root Signature are specified as following in the metadata:
-
-      !dx.rootsignatures = !{!2} ; list of function/root signature pairs
-      !2 = !{ ptr @main, !3 } ; function, root signature
-      !3 = !{ !4, !5, !6, !7 } ; list of root signature elements
-
-      So for each MDNode inside dx.rootsignatures NamedMDNode
-      (the Root parameter of this function), the parsing process needs
-      to loop through each of its operands and process the function,
-      signature pair.
-   */
-
-  // Get the Root Signature Description from the function signature pair.
-
   // Loop through the Root Elements of the root signature.
   for (const auto &Operand : Node->operands()) {
     MDNode *Element = dyn_cast<MDNode>(Operand);
@@ -114,6 +100,18 @@ static bool validate(LLVMContext *Ctx, const ModuleRootSignature &MRS) {
 static SmallDenseMap<const Function *, ModuleRootSignature>
 analyzeModule(Module &M) {
 
+  /** Root Signature are specified as following in the metadata:
+
+    !dx.rootsignatures = !{!2} ; list of function/root signature pairs
+    !2 = !{ ptr @main, !3 } ; function, root signature
+    !3 = !{ !4, !5, !6, !7 } ; list of root signature elements
+
+    So for each MDNode inside dx.rootsignatures NamedMDNode
+    (the Root parameter of this function), the parsing process needs
+    to loop through each of its operands and process the function,
+    signature pair.
+ */
+
   LLVMContext *Ctx = &M.getContext();
 
   SmallDenseMap<const Function *, ModuleRootSignature> MRSMap;
@@ -129,22 +127,22 @@ analyzeModule(Module &M) {
       continue;
     }
 
+    // Function was pruned during compilation.
     const MDOperand &FunctionPointerMdNode = RSDefNode->getOperand(0);
     if (FunctionPointerMdNode == nullptr) {
-      // Function was pruned during compilation.
       continue;
     }
 
     ValueAsMetadata *VAM =
         llvm::dyn_cast<ValueAsMetadata>(FunctionPointerMdNode.get());
     if (VAM == nullptr) {
-      reportError(Ctx, "First element of root signature is not a value");
+      reportError(Ctx, "First element of root signature is not a Value");
       continue;
     }
 
     Function *F = dyn_cast<Function>(VAM->getValue());
     if (F == nullptr) {
-      reportError(Ctx, "First element of root signature is not a function");
+      reportError(Ctx, "First element of root signature is not a Function");
       continue;
     }
 

llvm/lib/Target/DirectX/DXILRootSignature.h

@@ -18,7 +18,6 @@
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Pass.h"
-#include <optional>
 
 namespace llvm {
 namespace dxil {

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

@@ -2,7 +2,7 @@
 
 target triple = "dxil-unknown-shadermodel6.0-compute"
 
-; CHECK: error: Invalid Root Element: NOTRootFlags
+; CHECK: error: Invalid Root Signature Element: NOTRootFlags
 ; CHECK-NO: Root Signature Definitions
 
 

llvm/unittests/Object/DXContainerTest.cpp

@@ -870,8 +870,9 @@ TEST(RootSignature, ParseRootFlags) {
         0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
         0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
     };
-    EXPECT_THAT_EXPECTED(DXContainer::create(getMemoryBuffer<68>(Buffer)),
-                         FailedWithMessage("Invalid Root Signature Version"));
+    EXPECT_THAT_EXPECTED(
+        DXContainer::create(getMemoryBuffer<100>(Buffer)),
+        FailedWithMessage("unsupported root signature version read: 3"));
   }
   {
     // Flag has been set to an invalid value
@@ -883,7 +884,9 @@ TEST(RootSignature, ParseRootFlags) {
         0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
         0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0xFF,
     };
-    EXPECT_THAT_EXPECTED(DXContainer::create(getMemoryBuffer<68>(Buffer)),
-                         FailedWithMessage("Invalid Root Signature flag"));
+    EXPECT_THAT_EXPECTED(
+        DXContainer::create(getMemoryBuffer<100>(Buffer)),
+        FailedWithMessage(
+            "unsupported root signature flag value read: 4278190081"));
   }
 }