[PATCH] D29948: [AMDGPU] Restructure runtime metadata creation

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:277
       // Construct from an YAML string.
-      explicit Metadata(const std::string &YAML);
+      explicit Metadata(const std::string &YamlString);
 
----------------
Should be StringRef?


================
Comment at: lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:283
       // Convert from YAML string.
-      static Metadata fromYAML(const std::string &S);
+      static Metadata fromYamlString(const std::string &YamlString);
     };
----------------
Ditto


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.cpp:41-50
+namespace {
+
+cl::opt<bool> DumpRuntimeMetadata(
+    "amdgpu-rtmd-dump",
+    cl::desc("Dump AMDGPU Runtime Metadata"));
+cl::opt<bool> VerifyRuntimeMetadata(
+    "amdgpu-rtmd-verify", cl::Hidden,
----------------
Normally these are static and anonymous namespaces not used.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.cpp:154
+         << (YamlString == ToYamlString ? "PASS" : "FAIL") << '\n';
+  if (YamlString != ToYamlString)
+    errs() << "Original input: " << YamlString << '\n'
----------------
Braces


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.cpp:172-173
+    return KernelArg::Region;
+  default:
+    return KernelArg::Private;
+  }
----------------
I think private should be explicitly handled and default to an error value


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.cpp:279
+  auto &PI = Program.PrintfInfo;
+  if (auto Node = Mod.getNamedMetadata("llvm.printf.fmts"))
+    for (auto Op : Node->operands())
----------------
Early return to reduce indentation


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.h:1
+//===--- AMDGPURuntimeMetadataStreamer.h - Streams Runtime Metadata -------===//
+//
----------------
Missing c++ mode comment


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.h:15
+#include "llvm/ADT/StringRef.h"
+#include <cstdint>
+#include <string>
----------------
You shouldn't need to include stdint directly. You might want llvm/Support/DataTypes.h instead of you need it


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.h:36
+  typedef KernelArg::ValueType ValueType;
+  typedef std::vector<uint32_t> WorkGroupDimensions;
+
----------------
Isn't this limited to just 3? std::array?


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.h:40
+
+  void dump(const std::string &YamlString) const;
+
----------------
StringRef


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.h:42
+
+  void verify(const std::string &YamlString) const;
+
----------------
StringRef


https://reviews.llvm.org/D29948