[PATCH] D21849: [AMDGPU] Add metadata for runtime

[Matt Arsenault via llvm-commits]

arsenm added inline comments.

================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:800
@@ +799,3 @@
+    if (!isSigned)
+      return Twine("u") + getOCLTypeName(Ty, true);
+    auto IntTy = cast<IntegerType>(Ty);
----------------
Twine('u')

================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:823-826
@@ +822,6 @@
+         Type *Ty, StringRef TypeName) {
+  if (isa<VectorType>(Ty))
+    return getRuntimeMDValueType(Ty->getVectorElementType(), TypeName);
+  else if (isa<PointerType>(Ty))
+    return getRuntimeMDValueType(Ty->getPointerElementType(), TypeName);
+  else if (Ty->isHalfTy())
----------------
dyn_cast and use casted pointer

================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:831-849
@@ +830,21 @@
+    return RuntimeMD::KernelArg::F32;
+  else if (Ty->isDoubleTy())
+    return RuntimeMD::KernelArg::F64;
+  else if (IntegerType* intTy = dyn_cast<IntegerType>(Ty)) {
+    bool Signed = !TypeName.startswith("u");
+    switch (intTy->getIntegerBitWidth()) {
+    case 8:
+      return Signed ? RuntimeMD::KernelArg::I8 : RuntimeMD::KernelArg::U8;
+    case 16:
+      return Signed ? RuntimeMD::KernelArg::I16 : RuntimeMD::KernelArg::U16;
+    case 32:
+      return Signed ? RuntimeMD::KernelArg::I32 : RuntimeMD::KernelArg::U32;
+    case 64:
+      return Signed ? RuntimeMD::KernelArg::I64 : RuntimeMD::KernelArg::U64;
+    default:
+      // Runtime does not recognize other integer types. Report as
+      // struct type.
+      return RuntimeMD::KernelArg::Struct;
+    }
+  } else
+    return RuntimeMD::KernelArg::Struct;
----------------
There's no maximum , only when trying to use all strings on a single Cases call

================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:863
@@ +862,3 @@
+
+  for (auto &Arg:F.args()) {
+    // Emit KeyArgBegin.
----------------
Spaces around :

================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:946-948
@@ +945,5 @@
+    // Emit KeyArgAddrQual.
+    if (isa<PointerType>(T))
+      emitRuntimeMDIntValue(OutStreamer, RuntimeMD::KeyArgAddrQual,
+                            T->getPointerAddressSpace(), 1);
+
----------------
dyn_cast to PointerType and PT->getAddressSpace()

================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:955-957
@@ +954,5 @@
+  // Emit KeyReqdWorkGroupSize, KeyWorkGroupSizeHint, and KeyVecTypeHint.
+  auto RWGS = F.getMetadata("reqd_work_group_size");
+  auto WGSH = F.getMetadata("work_group_size_hint");
+  auto VTH = F.getMetadata("vec_type_hint");
+  if (RWGS)
----------------
These can be moved such that if (auto RWGS = ..)

================
Comment at: lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:1
@@ +1,2 @@
+//===-- AMDGPURuntimeMetadata.h - AMDGPU Runtime Metadata Header File -----===//
+//
----------------
Missing C++ mode comment

================
Comment at: lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:34
@@ +33,3 @@
+///
+/// v1.0: Created.
+//
----------------
Pointless comment

================
Comment at: lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:50
@@ +49,3 @@
+  // ELF section name containing runtime metadata
+  const char SectionName[] = ".AMDGPU.runtime_metadata";
+
----------------
global should not defined in a header

================
Comment at: lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:88
@@ +87,3 @@
+
+  enum Language : char {
+    OpenCL_C      = 0,
----------------
arsenm wrote:
> should we re-use dwarf language IDs + extensions or something instead of inventing a new enum? Maybe an unknown value for 0 or -1?
Specify the signedness, uint8_t

================
Comment at: lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:88-93
@@ +87,8 @@
+
+  enum Language : char {
+    OpenCL_C      = 0,
+    HCC           = 1,
+    OpenMP        = 2,
+    OpenCL_CPP    = 3,
+  };
+
----------------
should we re-use dwarf language IDs + extensions or something instead of inventing a new enum? Maybe an unknown value for 0 or -1?

================
Comment at: lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:95
@@ +94,3 @@
+
+  enum LanguageVersion : short {
+    V100          = 100,
----------------
Specify signedness and size, uint16_t

================
Comment at: lib/Target/AMDGPU/AMDGPURuntimeMetadata.h:104
@@ +103,3 @@
+  namespace KernelArg {
+    enum TypeKind : char {
+      Value     = 0,
----------------
Ditto for the rest of the enum types

================
Comment at: test/CodeGen/AMDGPU/runtime-metadata.ll:718
@@ +717,3 @@
+!50 = !{i32 1, i32 2, i32 3}
+!51 = !{!"int *", !"int *", !"int *"}
+!60 = !{i32 1, i32 1, i32 1}
----------------
Are these supposed to be missing the address space?

================
Comment at: test/CodeGen/AMDGPU/runtime-metadata.ll:722
@@ +721,2 @@
+!62 = !{!"image1d_t", !"image2d_t", !"image3d_t"}
+!70 = !{!"volatile", !"const restrict", !"pipe"}
----------------
A couple more tests with pointers to pointers, struct with a pointer, vectors of pointers, and an unknown string for a builtin type name


https://reviews.llvm.org/D21849