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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

I only consider the clSetKernelArg as a remaining bigger issue. Having only "polly_"-prefixed function non-static would also be great.

Tobias is the sole author of Polly-ACC. I think he should give the final LGTM.



================
Comment at: include/polly/LinkAllPasses.h:51
 #ifdef GPU_CODEGEN
-llvm::Pass *createPPCGCodeGenerationPass();
+llvm::Pass *createPPCGCodeGenerationPass(int Runtime);
 #endif
----------------
etherzhhb wrote:
> PhilippSchaad wrote:
> > etherzhhb wrote:
> > > is this Runtime supposed to be with type GPURuntimeT ? it is a little bit tricky here.
> > > Maybe we need to introduce a PPCG header and define the runtime enum there, than include that runtime enum.
> > > 
> > > or we can declare the function as
> > > 
> > > 
> > > ```
> > > llvm::Pass *createPPCGCodeGenerationPass(int Runtime = 0);
> > > ```
> > > 
> > > to at least avoid the magic number 0 in line 86.
> > > 
> > > 
> > Yes, it would be. The reason it's not is exactly the one you mentioned. I was considering adding a PPCG header, but refrained from it because I was hesitant about creating a header 'just for one enum'. If you agree that this is a good solution, I will indeed introduce a new header for PPCG and define the enum there, to get rid of magic numbers. The second option seems reasonable too though.
> we could start from the second option if you think it is reasonable
I assume you kept the arguments of type `int` to not include header files here.


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:769-772
+  if (Runtime == GPURuntime::CUDA)
+    Name = "polly_initContextCUDA";
+  else if (Runtime == GPURuntime::OpenCL)
+    Name = "polly_initContextCL";
----------------
Can you make this a switch so you get warned by the compiler when adding more runtimes?


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1726-1729
+    if (Runtime == GPURuntime::CUDA)
+      GPUTriple = llvm::Triple(Triple::normalize("nvptx64-nvidia-cuda"));
+    else if (Runtime == GPURuntime::OpenCL)
+      GPUTriple = llvm::Triple(Triple::normalize("nvptx64-nvidia-nvcl"));
----------------
Could also be a switch.


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:2689
+    break;
+  }
+
----------------
We should check whether the arguments are valid. Such as:
```
switch (Runtime) {
case 1:
case 2:
  ...
default:
  llvm_unreachable("Invalid argument for Runtime");
}
```


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:2691-2695
+  switch (Arch) {
+  case 1:
+    generator->Architecture = GPUArch::NVPTX64;
+    break;
+  }
----------------
Similarly:
```
switch (Arch) {
case 1:
  ...
default:
  llvm_unreachable("Invalid argument for Arch");
}
```


================
Comment at: lib/Support/RegisterPasses.cpp:331-348
+    int Arch;
+    int Runtime;
+
+    static const int UseOpenCLRuntime = 2;
+    Runtime = UseOpenCLRuntime;
+
+    switch (GPUArch) {
----------------
```
  int Arch;
  switch (GPUArch) {
  case GPU_ARCH_NVPTX64;
    Arch = 1;
    break;
  }

  int Runtime;
  switch (GPURuntime) {
  case GPU_RUNTIME_CUDA:
    Runtime = 1;
    break;
  case GPU_RUNTIME_OPENCL:
    Runtime = 2;
    breal
  }

  PM.add(polly::createPPCGCodeGenerationPass(Arch, Runtime))
```

With "you don't need a switch anymore", I was thinking about the like of:
```
  PM.add(polly::createPPCGCodeGenerationPass(1, GPURuntime + 1));
```
Your choice.

It could be helpful to have `createPPCGCodeGenerationPass` accept the GPURuntime enum as arguments instead.

In your solution, I don't see the use of `static const int` local variables. If you want identifiers that give names to the accepted arguments, declare them as `#define` or `static const int` in the header file that also declares `createPPCGCodeGenerationPass`, so these can be used in the implementation of `createPPCGCodeGenerationPass` as well.


================
Comment at: tools/GPURuntime/GPUJIT.c:397-401
+  if (Ret != CL_SUCCESS) {
+    fprintf(stdout, "Failed to create command queue.\n");
+    cl_printError(Ret);
+    exit(-1);
+  }
----------------
Did you consider introducing a new function this sequence of code? It appears quite often.


================
Comment at: tools/GPURuntime/GPUJIT.c:610-618
+    Ret = clSetKernelArgFcnPtr(CLKernel->Kernel, i, 8, (void *)Parameters[i]);
+    if (Ret == CL_INVALID_ARG_SIZE) {
+      Ret = clSetKernelArgFcnPtr(CLKernel->Kernel, i, 4, (void *)Parameters[i]);
+      if (Ret == CL_INVALID_ARG_SIZE) {
+        Ret =
+            clSetKernelArgFcnPtr(CLKernel->Kernel, i, 2, (void *)Parameters[i]);
+        if (Ret == CL_INVALID_ARG_SIZE) {
----------------
Trying each argument size after the other and hoping one matches is not good. The caller must know the argument sizes. You probably have to pass the sizes in another argument to `launchKernelCL` that contains those sizes for each argument, generated by Polly.

Without this, the code will fail if you pass a struct (or vector) of size other than 8, 4, 2, or 1.


================
Comment at: tools/GPURuntime/GPUJIT.c:676
+  if (DevData == 0) {
+    fprintf(stdout, "Allocate memory for GPU device memory pointer failed.\n");
+    exit(-1);
----------------
Shouldn't these print to `stderr`?


================
Comment at: tools/GPURuntime/GPUJIT.c:750
+
+void cl_printError(int Error) {
+
----------------
The function name does not follow the naming of other functions in this file. In C it is common have the public API functions prefixed with the library name (here: "polly") and everything else static. Don't choose the prefix of another library (here: "cl_"). This avoids symbol conflicts because multiple libraries happen to give the same name for a function.


https://reviews.llvm.org/D32431