Switch metadata generation on shader model instead of validator version.

- MinVal[Major|Minor] tracks shader model.
- Account for reflection by special casing for version 0.0 (no validation)
- Tolerate additions to non-critical metadata for future version
- Keep track of unrecognized non-critical metadata for validation
- Update validator to detect this case.

include/dxc/DXIL/DxilMetadataHelper.h

@@ -311,6 +311,7 @@ public:
   public:
     unsigned m_ValMajor, m_ValMinor;        // Reported validation version in DXIL
     unsigned m_MinValMajor, m_MinValMinor;  // Minimum validation version dictated by shader model
+    bool m_bExtraMetadata;
   };
 
 public:
@@ -423,6 +424,9 @@ public:
   llvm::Metadata *EmitSubobject(const DxilSubobject &obj);
   void LoadSubobject(const llvm::MDNode &MDO, DxilSubobjects &Subobjects);
 
+  // Extra metadata present
+  bool HasExtraMetadata() { return m_bExtraMetadata; }
+
   // Shader specific.
 private:
   llvm::MDTuple *EmitDxilGSState(DXIL::InputPrimitive Primitive, unsigned MaxVertexCount, 
@@ -508,6 +512,10 @@ private:
   std::unique_ptr<ExtraPropertyHelper> m_ExtraPropertyHelper;
   unsigned m_ValMajor, m_ValMinor;        // Reported validation version in DXIL
   unsigned m_MinValMajor, m_MinValMinor;  // Minimum validation version dictated by shader model
+
+  // Non-fatal if extra metadata is found, but will fail validation.
+  // This is how metadata can be exteneded.
+  bool m_bExtraMetadata;
 };
 
 

include/dxc/DXIL/DxilModule.h

@@ -178,6 +178,8 @@ public:
   void ReEmitDxilResources();
   /// Deserialize DXIL metadata form into in-memory form.
   void LoadDxilMetadata();
+  /// Return true if non-fatal metadata error was detected.
+  bool HasMetadataErrors();
 
   /// Check if a Named meta data node is known by dxil module.
   static bool IsKnownNamedMetaData(llvm::NamedMDNode &Node);
@@ -364,6 +366,10 @@ private:
   uint32_t m_AutoBindingSpace;
 
   std::unique_ptr<DxilSubobjects> m_pSubobjects;
+
+  // m_bMetadataErrors is true if non-fatal metadata errors were encountered.
+  // Validator will fail in this case, but should not block module load.
+  bool m_bMetadataErrors;
 };
 
 } // namespace hlsl

lib/DXIL/DxilMetadataHelper.cpp

@@ -84,6 +84,7 @@ DxilMDHelper::DxilMDHelper(Module *pModule, std::unique_ptr<ExtraPropertyHelper>
 , m_ValMinor(0)
 , m_MinValMajor(1)
 , m_MinValMinor(0)
+, m_bExtraMetadata(false)
 {
 }
 
@@ -97,6 +98,14 @@ void DxilMDHelper::SetShaderModel(const ShaderModel *pSM) {
     m_ValMajor = m_MinValMajor;
     m_ValMinor = m_MinValMinor;
   }
+  // Validator version 0.0 is not meant for validation or retail driver consumption,
+  // and is used for separate reflection.
+  // MinVal version drives metadata decisions for compatilbility, so snap this to
+  // latest for reflection/no validation case.
+  if (DXIL::CompareVersions(m_ValMajor, m_ValMinor, 0, 0) == 0) {
+    m_MinValMajor = 0;
+    m_MinValMinor = 0;
+  }
   if (m_ExtraPropertyHelper) {
     m_ExtraPropertyHelper->m_ValMajor = m_ValMajor;
     m_ExtraPropertyHelper->m_ValMinor = m_ValMinor;
@@ -509,7 +518,9 @@ void DxilMDHelper::LoadSignatureElement(const MDOperand &MDO, DxilSignatureEleme
     SE.SetSemanticIndexVec({0});
   }
   // Name-value list of extended properties.
+  m_ExtraPropertyHelper->m_bExtraMetadata = false;
   m_ExtraPropertyHelper->LoadSignatureElementProperties(pTupleMD->getOperand(kDxilSignatureElementNameValueList), SE);
+  m_bExtraMetadata |= m_ExtraPropertyHelper->m_bExtraMetadata;
 }
 
 //
@@ -627,7 +638,9 @@ void DxilMDHelper::LoadDxilSRV(const MDOperand &MDO, DxilResource &SRV) {
   SRV.SetSampleCount(ConstMDToUint32(pTupleMD->getOperand(kDxilSRVSampleCount)));
 
   // Name-value list of extended properties.
+  m_ExtraPropertyHelper->m_bExtraMetadata = false;
   m_ExtraPropertyHelper->LoadSRVProperties(pTupleMD->getOperand(kDxilSRVNameValueList), SRV);
+  m_bExtraMetadata |= m_ExtraPropertyHelper->m_bExtraMetadata;
 }
 
 MDTuple *DxilMDHelper::EmitDxilUAV(const DxilResource &UAV) {
@@ -669,7 +682,9 @@ void DxilMDHelper::LoadDxilUAV(const MDOperand &MDO, DxilResource &UAV) {
   UAV.SetROV(ConstMDToBool(pTupleMD->getOperand(kDxilUAVRasterizerOrderedView)));
 
   // Name-value list of extended properties.
+  m_ExtraPropertyHelper->m_bExtraMetadata = false;
   m_ExtraPropertyHelper->LoadUAVProperties(pTupleMD->getOperand(kDxilUAVNameValueList), UAV);
+  m_bExtraMetadata |= m_ExtraPropertyHelper->m_bExtraMetadata;
 }
 
 MDTuple *DxilMDHelper::EmitDxilCBuffer(const DxilCBuffer &CB) {
@@ -704,7 +719,9 @@ void DxilMDHelper::LoadDxilCBuffer(const MDOperand &MDO, DxilCBuffer &CB) {
   CB.SetSize(ConstMDToUint32(pTupleMD->getOperand(kDxilCBufferSizeInBytes)));
 
   // Name-value list of extended properties.
+  m_ExtraPropertyHelper->m_bExtraMetadata = false;
   m_ExtraPropertyHelper->LoadCBufferProperties(pTupleMD->getOperand(kDxilCBufferNameValueList), CB);
+  m_bExtraMetadata |= m_ExtraPropertyHelper->m_bExtraMetadata;
 }
 
 void DxilMDHelper::EmitDxilTypeSystem(DxilTypeSystem &TypeSystem, vector<GlobalVariable*> &LLVMUsed) {
@@ -826,11 +843,15 @@ void DxilMDHelper::LoadDxilTemplateArgAnnotation(const llvm::MDOperand &MDO, Dxi
     IFTBOOL(pTupleMD->getNumOperands() == 2, DXC_E_INCORRECT_DXIL_METADATA);
     annotation.SetIntegral((int64_t)ConstMDToUint64(pTupleMD->getOperand(kDxilTemplateArgValue)));
     break;
+  default:
+    DXASSERT(false, "Unknown template argument type tag.");
+    m_bExtraMetadata = true;
+    break;
   }
 }
 
 Metadata *DxilMDHelper::EmitDxilStructAnnotation(const DxilStructAnnotation &SA) {
-  bool bSupportExtended = DXIL::CompareVersions(m_ValMajor, m_ValMinor, 1, 5) >= 0;
+  bool bSupportExtended = DXIL::CompareVersions(m_MinValMajor, m_MinValMinor, 1, 5) >= 0;
 
   vector<Metadata *> MDVals;
   MDVals.reserve(SA.GetNumFields() + 2);  // In case of extended 1.5 property list
@@ -863,20 +884,32 @@ void DxilMDHelper::LoadDxilStructAnnotation(const MDOperand &MDO, DxilStructAnno
   if (pTupleMD->getNumOperands() == 1) {
     SA.MarkEmptyStruct();
   }
-  if (DXIL::CompareVersions(m_ValMajor, m_ValMinor, 1, 5) >= 0 &&
-      (pTupleMD->getNumOperands() == SA.GetNumFields()+2)) {
+  if (pTupleMD->getNumOperands() == SA.GetNumFields()+2) {
+    DXASSERT(DXIL::CompareVersions(m_MinValMajor, m_MinValMinor, 1, 5) >= 0,
+      "otherwise, template annotation emitted for dxil version < 1.5");
     // Load template args from extended operand
     const MDOperand &MDOExtra = pTupleMD->getOperand(SA.GetNumFields()+1);
     const MDTuple *pTupleMDExtra = dyn_cast_or_null<MDTuple>(MDOExtra.get());
     if(pTupleMDExtra) {
-      IFTBOOL(pTupleMDExtra->getNumOperands() % 2 == 0, DXC_E_INCORRECT_DXIL_METADATA);
-      unsigned Tag = ConstMDToUint32(pTupleMDExtra->getOperand(0));
-      IFTBOOL(Tag == kDxilTemplateArgumentsTag, DXC_E_INCORRECT_DXIL_METADATA); // Only one allowed at this point
-      const MDTuple *pTupleTemplateArgs = dyn_cast_or_null<MDTuple>(pTupleMDExtra->getOperand(1).get());
-      IFTBOOL(pTupleTemplateArgs, DXC_E_INCORRECT_DXIL_METADATA);
-      SA.SetNumTemplateArgs(pTupleTemplateArgs->getNumOperands());
-      for (unsigned i = 0; i < pTupleTemplateArgs->getNumOperands(); ++i) {
-        LoadDxilTemplateArgAnnotation(pTupleTemplateArgs->getOperand(i), SA.GetTemplateArgAnnotation(i));
+      for (unsigned i = 0; i < pTupleMDExtra->getNumOperands(); i += 2) {
+        unsigned Tag = ConstMDToUint32(pTupleMDExtra->getOperand(i));
+        const MDOperand &MDO = pTupleMDExtra->getOperand(i + 1);
+        IFTBOOL(MDO.get() != nullptr, DXC_E_INCORRECT_DXIL_METADATA);
+
+        switch (Tag) {
+        case kDxilTemplateArgumentsTag: {
+          const MDTuple *pTupleTemplateArgs = dyn_cast_or_null<MDTuple>(pTupleMDExtra->getOperand(1).get());
+          IFTBOOL(pTupleTemplateArgs, DXC_E_INCORRECT_DXIL_METADATA);
+          SA.SetNumTemplateArgs(pTupleTemplateArgs->getNumOperands());
+          for (unsigned i = 0; i < pTupleTemplateArgs->getNumOperands(); ++i) {
+            LoadDxilTemplateArgAnnotation(pTupleTemplateArgs->getOperand(i), SA.GetTemplateArgAnnotation(i));
+          }
+        } break;
+        default:
+          DXASSERT(false, "unknown extended tag for struct annotation.");
+          m_bExtraMetadata = true;
+          break;
+        }
       }
     }
   } else {
@@ -989,7 +1022,7 @@ Metadata *DxilMDHelper::EmitDxilFieldAnnotation(const DxilFieldAnnotation &FA) {
     MDVals.emplace_back(Uint32ToConstMD((unsigned)FA.GetCompType().GetKind()));
   }
   if (FA.IsCBVarUsed() &&
-      DXIL::CompareVersions(m_ValMajor, m_ValMinor, 1, 5) >= 0) {
+      DXIL::CompareVersions(m_MinValMajor, m_MinValMinor, 1, 5) >= 0) {
     MDVals.emplace_back(Uint32ToConstMD(kDxilFieldAnnotationCBUsedTag));
     MDVals.emplace_back(BoolToConstMD(true));
   }
@@ -1041,12 +1074,9 @@ void DxilMDHelper::LoadDxilFieldAnnotation(const MDOperand &MDO, DxilFieldAnnota
       FA.SetCBVarUsed(ConstMDToBool(MDO));
       break;
     default:
-      // TODO:  I don't think we should be failing unrecognized extended tags.
-      //        Perhaps we can flag this case in the module and fail validation
-      //        if flagged.
-      //        That way, an existing loader will not fail on an additional tag
-      //        and the blob would not be signed if the extra tag was not legal.
-      IFTBOOL(false, DXC_E_INCORRECT_DXIL_METADATA);
+      DXASSERT(false, "Unknown extended shader properties tag");
+      m_bExtraMetadata = true;
+      break;
     }
   }
 }
@@ -1393,6 +1423,7 @@ void DxilMDHelper::LoadDxilEntryProperties(const MDOperand &MDO,
     } break;
     default:
       DXASSERT(false, "Unknown extended shader properties tag");
+      m_bExtraMetadata = true;
       break;
     }
   }
@@ -1666,6 +1697,7 @@ Metadata *DxilMDHelper::EmitSubobject(const DxilSubobject &obj) {
   }
   default:
     DXASSERT(false, "otherwise, we didn't handle a valid subobject kind");
+    m_bExtraMetadata = true;
     break;
   }
   return MDNode::get(m_Ctx, Args);
@@ -1745,6 +1777,7 @@ void DxilMDHelper::LoadSubobject(const llvm::MDNode &MD, DxilSubobjects &Subobje
   }
   default:
     DXASSERT(false, "otherwise, we didn't handle a valid subobject kind");
+    m_bExtraMetadata = true;
     break;
   }
 }
@@ -1780,7 +1813,9 @@ void DxilMDHelper::LoadDxilSampler(const MDOperand &MDO, DxilSampler &S) {
   S.SetSamplerKind((DxilSampler::SamplerKind)ConstMDToUint32(pTupleMD->getOperand(kDxilSamplerType)));
 
   // Name-value list of extended properties.
+  m_ExtraPropertyHelper->m_bExtraMetadata = false;
   m_ExtraPropertyHelper->LoadSamplerProperties(pTupleMD->getOperand(kDxilSamplerNameValueList), S);
+  m_bExtraMetadata |= m_ExtraPropertyHelper->m_bExtraMetadata;
 }
 
 const MDOperand &DxilMDHelper::GetResourceClass(llvm::MDNode *MD,
@@ -2026,7 +2061,8 @@ void DxilMDHelper::LoadDxilASState(const MDOperand &MDO, unsigned *NumThreads, u
 //
 DxilMDHelper::ExtraPropertyHelper::ExtraPropertyHelper(Module *pModule)
 : m_Ctx(pModule->getContext())
-, m_pModule(pModule) {
+, m_pModule(pModule)
+, m_bExtraMetadata(false) {
 }
 
 DxilExtraPropertyHelper::DxilExtraPropertyHelper(Module *pModule)
@@ -2073,6 +2109,8 @@ void DxilExtraPropertyHelper::LoadSRVProperties(const MDOperand &MDO, DxilResour
       break;
     default:
       DXASSERT(false, "Unknown resource record tag");
+      m_bExtraMetadata = true;
+      break;
     }
   }
 }
@@ -2126,6 +2164,8 @@ void DxilExtraPropertyHelper::LoadUAVProperties(const MDOperand &MDO, DxilResour
       break;
     default:
       DXASSERT(false, "Unknown resource record tag");
+      m_bExtraMetadata = true;
+      break;
     }
   }
 }
@@ -2160,6 +2200,8 @@ void DxilExtraPropertyHelper::LoadCBufferProperties(const MDOperand &MDO, DxilCB
       break;
     default:
       DXASSERT(false, "Unknown cbuffer tag");
+      m_bExtraMetadata = true;
+      break;
     }
   }
 }
@@ -2188,6 +2230,8 @@ void DxilExtraPropertyHelper::EmitSignatureElementProperties(const DxilSignature
 
   if (SE.GetUsageMask() != 0 &&
       DXIL::CompareVersions(m_ValMajor, m_ValMinor, 1, 5) >= 0) {
+    // Emitting this will not hurt old reatil loader (only asserts),
+    // and is required for signatures to match in validation.
     MDVals.emplace_back(DxilMDHelper::Uint32ToConstMD(DxilMDHelper::kDxilSignatureElementUsageCompMaskTag, m_Ctx));
     MDVals.emplace_back(DxilMDHelper::Uint32ToConstMD(SE.GetUsageMask(), m_Ctx));
   }
@@ -2220,6 +2264,8 @@ void DxilExtraPropertyHelper::LoadSignatureElementProperties(const MDOperand &MD
       break;
     default:
       DXASSERT(false, "Unknown signature element tag");
+      m_bExtraMetadata = true;
+      break;
     }
   }
 }

lib/DXIL/DxilModule.cpp

@@ -115,6 +115,7 @@ DxilModule::DxilModule(Module *pModule)
 , m_IntermediateFlags(0)
 , m_AutoBindingSpace(UINT_MAX)
 , m_pSubobjects(nullptr)
+, m_bMetadataErrors(false)
 {
 
   DXASSERT_NOMSG(m_pModule != nullptr);
@@ -1389,7 +1390,12 @@ bool DxilModule::IsKnownNamedMetaData(llvm::NamedMDNode &Node) {
   return DxilMDHelper::IsKnownNamedMetaData(Node);
 }
 
+bool DxilModule::HasMetadataErrors() {
+  return m_bMetadataErrors;
+}
+
 void DxilModule::LoadDxilMetadata() {
+  m_bMetadataErrors = false;
   m_pMDHelper->LoadDxilVersion(m_DxilMajor, m_DxilMinor);
   m_pMDHelper->LoadValidatorVersion(m_ValMajor, m_ValMinor);
   const ShaderModel *loadedSM;
@@ -1480,11 +1486,23 @@ void DxilModule::LoadDxilMetadata() {
 
   LoadDxilResources(*pEntryResources);
 
-  m_pMDHelper->LoadDxilTypeSystem(*m_pTypeSystem.get());
+  // Type system is not required for consumption of dxil.
+  try {
+    m_pMDHelper->LoadDxilTypeSystem(*m_pTypeSystem.get());
+  } catch (hlsl::Exception &) {
+    m_bMetadataErrors = true;
+#ifdef DBG
+    throw;
+#endif
+    m_pTypeSystem->GetStructAnnotationMap().clear();
+    m_pTypeSystem->GetFunctionAnnotationMap().clear();
+  }
 
   m_pMDHelper->LoadRootSignature(m_SerializedRootSignature);
 
   m_pMDHelper->LoadDxilViewIdState(m_SerializedState);
+
+  m_bMetadataErrors |= m_pMDHelper->HasExtraMetadata();
 }
 
 MDTuple *DxilModule::EmitDxilResources() {

lib/DxilContainer/DxilContainerAssembler.cpp

@@ -1693,21 +1693,17 @@ void hlsl::SerializeDxilContainerForModule(DxilModule *pModule,
   // Clone module for reflection, strip function defs
   std::unique_ptr<Module> reflectionModule;
   if (bEmitReflection) {
-    if (DXIL::CompareVersions(ValMajor, ValMinor, 1, 5) < 0) {
-      // Retain usage information in metadata for reflection by:
-      // Upgrade validator version, re-emit metadata, then clone module for reflection.
-      // 0,0 = Not meant to be validated, support latest
-      pModule->SetValidatorVersion(0, 0);
-      pModule->ReEmitDxilResources();
-    }
+    // Retain usage information in metadata for reflection by:
+    // Upgrade validator version, re-emit metadata, then clone module for reflection.
+    // 0,0 = Not meant to be validated, support latest
+    pModule->SetValidatorVersion(0, 0);
+    pModule->ReEmitDxilResources();
 
     reflectionModule.reset(llvm::CloneModule(pModule->GetModule()));
 
-    if (DXIL::CompareVersions(ValMajor, ValMinor, 1, 5) < 0) {
-      // Now restore validator version on main module and re-emit metadata.
-      pModule->SetValidatorVersion(ValMajor, ValMinor);
-      pModule->ReEmitDxilResources();
-    }
+    // Now restore validator version on main module and re-emit metadata.
+    pModule->SetValidatorVersion(ValMajor, ValMinor);
+    pModule->ReEmitDxilResources();
 
     for (Function &F : reflectionModule->functions()) {
       if (!F.isDeclaration()) {

lib/HLSL/DxilValidation.cpp

@@ -5617,6 +5617,10 @@ ValidateDxilModule(llvm::Module *pModule, llvm::Module *pDebugModule) {
   if (!pDxilModule) {
     return DXC_E_IR_VERIFICATION_FAILED;
   }
+  if (pDxilModule->HasMetadataErrors()) {
+    DiagPrinter << "Metadata error encountered in non-critical metadata (such as Type Annotations).\n";
+    return DXC_E_IR_VERIFICATION_FAILED;
+  }
 
   ValidationContext ValCtx(*pModule, pDebugModule, *pDxilModule, DiagPrinter);
 

tools/clang/test/HLSLFileCheck/d3dreflect/cb_sizes.hlsl

@@ -1,4 +1,4 @@
-// RUN: %dxilver 1.5 | %dxc -E main -T vs_6_0 %s | %D3DReflect %s | FileCheck %s
+// RUN: %dxc -E main -T vs_6_0 -Vd -validator-version 0.0 %s | %D3DReflect %s | FileCheck %s
 
 // Verify CB variable sizes align with expectations.
 // This also tests some matrix, struct, and array cases that may

tools/clang/test/HLSLFileCheck/d3dreflect/cbuf-usage-lib.hlsl

@@ -1,4 +1,4 @@
-// RUN: %dxilver 1.5 | %dxc -auto-binding-space 13 -T lib_6_3 %s | %D3DReflect %s | FileCheck %s
+// RUN: %dxc -auto-binding-space 13 -T lib_6_3 -Vd -validator-version 0.0 %s | %D3DReflect %s | FileCheck %s
 
 // Make sure usage flag is set properly for cbuffers used in libraries
 

tools/clang/test/HLSLFileCheck/d3dreflect/empty_struct2.hlsl

@@ -1,4 +1,4 @@
-// RUN: %dxilver 1.5 | %dxc -E main -T vs_6_0 %s | %D3DReflect %s | FileCheck %s
+// RUN: %dxc -E main -T vs_6_0 -Vd -validator-version 0.0 %s | %D3DReflect %s | FileCheck %s
 
 // Make sure nest empty struct works.
 

tools/clang/test/HLSLFileCheck/d3dreflect/lib_global.hlsl

@@ -1,4 +1,4 @@
-// RUN: %dxilver 1.5 | %dxc -T lib_6_3 -enable-16bit-types %s | %D3DReflect %s | FileCheck %s
+// RUN: %dxc -T lib_6_3 -enable-16bit-types -Vd -validator-version 0.0 %s | %D3DReflect %s | FileCheck %s
 
 // Note: validator version 1.5 is required because these tests use
 // module disassembly -> reassembly between steps, and type annotations

tools/clang/test/HLSLFileCheck/d3dreflect/reflect-lib-1.hlsl

@@ -1,4 +1,4 @@
-// RUN: %dxilver 1.5 | %dxc -T lib_6_3 -auto-binding-space 11 -default-linkage external %s | %D3DReflect %s | FileCheck %s
+// RUN: %dxc -T lib_6_3 -auto-binding-space 11 -default-linkage external -Vd -validator-version 0.0 %s | %D3DReflect %s | FileCheck %s
 
 float cbval1;
 cbuffer MyCB : register(b11, space2) { int4 cbval2, cbval3; }

tools/clang/test/HLSLFileCheck/d3dreflect/structured_buffer_layout.hlsl

@@ -1,4 +1,4 @@
-// RUN: %dxilver 1.5 | %dxc -E main -T vs_6_0 %s | %D3DReflect %s | FileCheck %s
+// RUN: %dxc -E main -T vs_6_0 -Vd -validator-version 0.0 %s | %D3DReflect %s | FileCheck %s
 
 // Verify SB type description does not follow the CB offseting alignment
 // even when structure is shared with a ConstantBuffer.