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

[Yaxun Liu via llvm-commits]

yaxunl added a comment.

In http://reviews.llvm.org/D21849#470373, @arsenm wrote:

> Needs more tests. I would like the full set of types to be tested, particularly weird ones like 3-vectors. Other non-OpenCL types like i1 or a random other bitwidth should also be tested. A few tests that use multiple arguments also would be useful


I added more tests. bool is not allowed as kernel argument, so I did not add test for bool.


================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:758
@@ +757,3 @@
+    default:
+      llvm_unreachable("invalid integer type");
+    }
----------------
arsenm wrote:
> This is reachable with valid IR. How about returning an empty string, or i + bitwidth?
I will return i+bitwidth

================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:831-849
@@ +830,21 @@
+      void setTypeKind(Type *T, StringRef TypeName) {
+        if (TypeName == "sampler_t")
+          TypeKind = (unsigned)KAT_Sampler;
+        else if (TypeName == "queue_t")
+          TypeKind = (unsigned)KAT_Queue;
+        else if (TypeName == "image1d_t" ||
+                 TypeName == "image1d_array_t" ||
+                 TypeName == "image1d_buffer_t" ||
+                 TypeName == "image2d_t" ||
+                 TypeName == "image2d_array_t" ||
+                 TypeName == "image2d_depth_t" ||
+                 TypeName == "image2d_array_depth_t" ||
+                 TypeName == "image2d_msaa_t" ||
+                 TypeName == "image2d_array_msaa_t" ||
+                 TypeName == "image2d_msaa_depth_t" ||
+                 TypeName == "image2d_array_msaa_depth_t" ||
+                 TypeName == "image3d_t")
+            TypeKind = (unsigned)KAT_Image;
+        else if (isa<PointerType>(T))
+          TypeKind = (unsigned)KAT_Pointer;
+        else
----------------
arsenm wrote:
> StringSwitch
There is else if condition not using string, and StringSwitch.Cases only allow 5 strings maximum.

================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:936-937
@@ +935,4 @@
+    Flag.setAccessQualifier(AccQual);
+    Flag.setAddressQualifier(isa<PointerType>(T) ?
+      T->getPointerAddressSpace() : 0);
+    Flag.setHasName(ArgNameMD);
----------------
arsenm wrote:
> It seems odd to specify 0 as the address pace for no address space since it's private. -1?
This bit fiels has only 2 bits, so the maximum value is 3, which will be the same as local addr space. This value should be ignored if the argument is not pointer, so it should not matter.


http://reviews.llvm.org/D21849