[PATCH] D32431: [Polly] Added OpenCL Runtime to GPURuntime Library for GPGPU CodeGen

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1562
+  else if (GPUNodeBuilder::Runtime == GPU_RUNTIME_OPENCL)
+    GPUModule->setTargetTriple(Triple::normalize("nvptx64-nvidia-nvcl"));
+
----------------
PhilippSchaad wrote:
> Meinersbur wrote:
> > Is there some vendor-neutral triple?
> Do you mean like `nvptx64-nvcl` / `nvptx64-cuda`?
I hoped that there might be some kind of triple that works for OpenCL in general, not only for nvidia (`nvptx`, `nvcl`). If the generated program only works for devices that support cuda anyway, I don't see where the benefit of such a backend is.

If there is indeed no backend that also works on non-nvidia devices, should we call the the runtime accordingly, e.g. "nvcl" then?


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:58-60
+  GPU_RUNTIME_NONE,
+  GPU_RUNTIME_CUDA,
+  GPU_RUNTIME_OPENCL
----------------
See [[ http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly ]] for LLVM's coding policy for enum members.

Nitpick: A "T" suffix is rather unusual.


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:156
                  DominatorTree &DT, Scop &S, BasicBlock *StartBlock,
-                 gpu_prog *Prog)
+                 gpu_prog *Prog, enum GPURuntimeT GPURuntime)
       : IslNodeBuilder(Builder, Annotator, DL, LI, SE, DT, S, StartBlock),
----------------
Nitpick: No need to use an `enum` qualifier.


================
Comment at: lib/Support/RegisterPasses.cpp:312-319
+  if (Target == TARGET_GPU_CUDA) {
 #ifdef GPU_CODEGEN
-    PM.add(polly::createPPCGCodeGenerationPass());
+    PM.add(polly::createPPCGCodeGenerationPassCUDA());
+#endif
+  } else if (Target == TARGET_GPU_OPENCL) {
+#ifdef GPU_CODEGEN
+    PM.add(polly::createPPCGCodeGenerationPassOpenCL());
----------------
Meinersbur wrote:
> A switch instead?
Now that `createPPCGCodeGenerationPass` takes an argument, you don't need a switch anymore.


================
Comment at: tools/GPURuntime/GPUJIT.c:303-311
+  cl_platform_id platform_id = NULL;
+  cl_device_id device_id = NULL;
+  cl_uint num_platforms;
+  cl_uint num_devices;
+  cl_int ret;
+
+  char DeviceRevision[256];
----------------
Consistent variable name style? What style do you intend to use in this file?


================
Comment at: tools/GPURuntime/GPUJIT.c:347-348
+  /* Get device revision. */
+  ret = clGetDeviceInfoFcnPtr(device_id, CL_DEVICE_VERSION, 256, DeviceRevision,
+                              &DeviceRevisionRetSize);
+  if (ret != CL_SUCCESS) {
----------------
Replace the magic number 256 by `sizeof(DeviceRevision)`?


https://reviews.llvm.org/D32431