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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

You changed `stdout` to `stderr` everywhere, which is better in my point of view, but logically is a different change. Sorry that I didn't realize that the libcudart also printed to stdout before so you tried to be consistent. Could you commit that change separately beforehand? (Maybe also the change in argument name capitalization)

I am accepting the patch proved that you are going to improve the `clSetKernelArg` situation later **and** add a TODO into the code about it. The other stuff is of stylistic nature only.

Please also wait for Tobias' approval.



================
Comment at: lib/Support/RegisterPasses.cpp:331-348
+    int Arch;
+    int Runtime;
+
+    static const int UseOpenCLRuntime = 2;
+    Runtime = UseOpenCLRuntime;
+
+    switch (GPUArch) {
----------------
PhilippSchaad wrote:
> Meinersbur wrote:
> > ```
> >   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.
> > With "you don't need a switch anymore", I was thinking about the like of:
> ```
> PM.add(polly::createPPCGCodeGenerationPass(1, GPURuntime + 1));
> ```
> > Your choice.
> Personally, I think the current solution is tiny little bit more 'documenting'. Both is fine though, good call.
> 
> 
> > It could be helpful to have createPPCGCodeGenerationPass accept the GPURuntime enum as arguments instead.
> That is true, would mean having to provide a header with that GPURuntime enum instead though, right?
> 
> > 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.
> Would it maybe make sense to introduce a PPCGCodeGeneration header at this point?
Please remove at least the `static` keyword. It makes sense for global constants, but not for function-local ones.

The style
```
static const int ArgumentName = 0;
func(ArgumentName);
```
rather unusual in LLVM-style code (but not bad if applied consistenly, which is unfortunately not the case in Polly). I've seen
```
func(/* ArgumentName = */ 0);
```
much more often.

In this case I think `UseOpenCLRuntime`, `UseCUDARuntime` and `TargetNVPTX64` should really be global constants declated close to the declaration of `createPPCGCodeGenerationPass` so it can be used by ever caller of that function.

> Would it maybe make sense to introduce a PPCGCodeGeneration header at this point?

Yes, that sounds good to me as well.


================
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) {
----------------
PhilippSchaad wrote:
> Meinersbur wrote:
> > 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.
> Yes, this is a priority issue still. The issue will have to be resolved at some point. This is basically a temporary way around some (probably) major argument handling changes in PPCG etc.
Also note that I am not sure that OpenCL ICD's are required to check for correct `CL_INVALID_ARG_SIZE`. It might just trust the caller, or be a badly written one.


================
Comment at: tools/GPURuntime/GPUJIT.c:606-607
+  if (!GlobalContext) {
+    fprintf(stderr, "GPGPU-code generation not correctly initialized.\n");
+    exit(-1);
+  }
----------------
Thanks for the introduction of `checkOpenCLError`. You could also introduce one for these two lines. for instance:
```
if (!GlobalContext)
  handleError("GPGPU-code generation not correctly initialized.\n");
```
`handleError` could also be called by `checkOpenCLError`. It helps centralising the error handling, such that if we change some detail about it (e.g. the return code on exit, or some cleanup code), there is a single function for that. 


================
Comment at: tools/GPURuntime/GPUJIT.c:958-963
+typedef CUresult CUDAAPI CuLinkAddDataFcnTy(CUlinkState State,
+                                            CUjitInputType Type, void *Data,
+                                            size_t Size, const char *Name,
+                                            unsigned int NumOptions,
+                                            CUjit_option *Options,
+                                            void **OptionValues);
----------------
These are unrelated changes Tobias usually complains about. I personally don't care.


================
Comment at: tools/GPURuntime/GPUJIT.c:1098
   dump_function();
+
   PollyGPUContext *Context;
----------------
Unrelated whitespace change?


https://reviews.llvm.org/D32431