[PATCH] D130207: [HLSL] Move DXIL validation version out of ModuleFlags

[Chris Bieneman via Phabricator via llvm-commits]

beanz added a comment.

A few nitpicks, otherwise this looks good.



================
Comment at: llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp:61
 
-static void cleanModuleFlags(Module &M) {
-  constexpr StringLiteral DeadKeys[] = {ValVerKey};
-  // Collect DeadKeys in ModuleFlags.
-  StringSet<> DeadKeySet;
-  for (auto &Key : DeadKeys) {
-    if (M.getModuleFlag(Key))
-      DeadKeySet.insert(Key);
-  }
-  if (DeadKeySet.empty())
-    return;
-
-  SmallVector<Module::ModuleFlagEntry, 8> ModuleFlags;
-  M.getModuleFlagsMetadata(ModuleFlags);
-  NamedMDNode *MDFlags = M.getModuleFlagsMetadata();
-  MDFlags->eraseFromParent();
-  // Add ModuleFlag which not dead.
-  for (auto &Flag : ModuleFlags) {
-    StringRef Key = Flag.Key->getString();
-    if (DeadKeySet.contains(Key))
-      continue;
-    M.addModuleFlag(Flag.Behavior, Key, Flag.Val);
-  }
-}
-
-static void cleanModule(Module &M) { cleanModuleFlags(M); }
+static void cleanModule(Module &M) {}
 
----------------
Better to remove this function if it is empty. If we need it in the future we can re-introduce it.


================
Comment at: llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp:80
 bool DXILTranslateMetadata::runOnModule(Module &M) {
-  if (MDNode *ValVerMD = cast_or_null<MDNode>(M.getModuleFlag(ValVerKey))) {
-    auto ValVer = loadDXILValidatorVersion(ValVerMD);
+  if (auto *ValVerMD = M.getNamedMetadata(ValVerKey)) {
+    auto ValVer = loadDXILValidatorVersion(ValVerMD->getOperand(0));
----------------
nit: LLVM's coding conventions discourage blanket use of `auto`. This one is a bit on the edge of the convention. My preference would be for this to have the type because I think that is more clear.

see: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp:81
+  if (auto *ValVerMD = M.getNamedMetadata(ValVerKey)) {
+    auto ValVer = loadDXILValidatorVersion(ValVerMD->getOperand(0));
     if (!ValVer.empty())
----------------
This should definitely have the type, not auto.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130207/new/

https://reviews.llvm.org/D130207