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

[Konstantin Zhuravlyov via Phabricator via llvm-commits]

kzhuravl added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:39
 
+namespace llvm {
 namespace AMDGPU {
----------------
kzhuravl wrote:
> yaxunl wrote:
> > This header file is shared between runtime and compiler. It may not be proper to use namespace llvm here.
> > 
> > Also, changes to this file requires corresponding changes in runtime. Are the runtime changes in place?
> I will probably switch away from llvm namespace here. I will update opencl runtime during the merge if needed.
We have llvm::AMDGPU namespace already. Having ::AMDGPU is causing problems... I thought about switching ::AMDGPU to AMD::AMDGPU, but not sure. I think having llvm::AMDGPU here is fine - it kind of tells that this header file originated and maintained by llvm?


================
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);
 
----------------
arsenm wrote:
> Should be StringRef?
I do not think we should use StringRef here. We want to share this header file with other components (ocl runtime, hcc runtime, offline tools), which might not have 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);
     };
----------------
arsenm wrote:
> Ditto
Same as above.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMetadataStreamer.h:36
+  typedef KernelArg::ValueType ValueType;
+  typedef std::vector<uint32_t> WorkGroupDimensions;
+
----------------
arsenm wrote:
> Isn't this limited to just 3? std::array?
Yes, but won't work with yaml.


https://reviews.llvm.org/D29948