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

[Philipp Schaad via Phabricator via llvm-commits]

PhilippSchaad added a comment.

Looking into the rest of your comments.



================
Comment at: include/polly/LinkAllPasses.h:51
 #ifdef GPU_CODEGEN
-llvm::Pass *createPPCGCodeGenerationPass();
+llvm::Pass *createPPCGCodeGenerationPass(int Runtime);
 #endif
----------------
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.


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1562
+  else if (GPUNodeBuilder::Runtime == GPU_RUNTIME_OPENCL)
+    GPUModule->setTargetTriple(Triple::normalize("nvptx64-nvidia-nvcl"));
+
----------------
Meinersbur wrote:
> etherzhhb wrote:
> > PhilippSchaad wrote:
> > > Meinersbur wrote:
> > > > Is there some vendor-neutral triple?
> > > Do you mean like `nvptx64-nvcl` / `nvptx64-cuda`?
> > for opencl, it can be "spir-unknown-unknown" or "spir64-unknown-unknown", but that may not work :)
> 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?
Looking into it. The next goal would be to add the AMDGPU backend to generate AMD ISA, which would then again utilize the same OpenCL Runtime implemented here. (I realize there will have to be some naming changes to make that clear in the `GPUJIT`, but as you pointed out, I have a naming-mess to fix there anyway.


https://reviews.llvm.org/D32431