[PATCH] End-to-end CUDA compilation.

[Eli Bendersky]

Where are the tests for emitting ctors/dtors, registering kernels, etc?


================
Comment at: include/clang/Driver/CC1Options.td:605
@@ -604,1 +604,3 @@
 
+def cuda_include_gpucode : Separate<["-"], "cuda-include-gpucode">,
+  HelpText<"Incorporate CUDA device-side code.">;
----------------
Should we prefix all cuda-related flags with -f for consistency with the existing ones? Don't know if it makes sense given that the cl_ ones above (for example) have no -f, but at least the CUDA ones should be consistent among themselves

================
Comment at: lib/CodeGen/CGCUDANV.cpp:38
@@ +37,3 @@
+  /// Convenience reference to LLVM Context
+  llvm::LLVMContext &VMContext;
+  /// Convenience reference to the current module
----------------
s/VMContext/Context/

================
Comment at: lib/CodeGen/CGCUDANV.cpp:41
@@ -35,1 +40,3 @@
+  llvm::Module &TheModule;
+  llvm::SmallVector<llvm::GlobalVariable *, 16> FatbinHandles;
 
----------------
Document FatbinHandles

================
Comment at: lib/CodeGen/CGCUDANV.cpp:88
@@ -56,1 +87,3 @@
+  VoidPtrPtrTy = VoidPtrTy->getPointerTo();
+
 }
----------------
extra line

================
Comment at: lib/CodeGen/CGCUDANV.cpp:170
@@ +169,3 @@
+
+  // void __cudaRegisterFunction(void **, const char *, char *, const char *,
+  //                             int, uint3*, uint3*, dim3*, dim3*, int*)
----------------
Can you include the parameter names in this declaration? It would be much easier to follow

I believe this comes from host_runtime.h?

================
Comment at: lib/CodeGen/CGCUDANV.cpp:179
@@ +178,3 @@
+
+  llvm::Argument &BlobHandlePtr = *RegisterKernelsFunc->arg_begin();
+  for (llvm::Function *Kernel : EmittedKernels) {
----------------
Please document what BlobHandlePtr means here and how it's used

================
Comment at: lib/CodeGen/CGCUDANV.cpp:241
@@ +240,3 @@
+    llvm::Constant *Values[] = {
+        llvm::ConstantInt::get(IntTy, 0x466243b1), // Fatbin wrapper magic.
+        llvm::ConstantInt::get(IntTy, 1),          // Fatbin version.
----------------
Use a named constant for the magic number -- it will then document itself


================
Comment at: lib/CodeGen/CGCUDANV.cpp:242
@@ +241,3 @@
+        llvm::ConstantInt::get(IntTy, 0x466243b1), // Fatbin wrapper magic.
+        llvm::ConstantInt::get(IntTy, 1),          // Fatbin version.
+        MakeConstantString(CodeOrErr.get()->getBuffer(), "", 16), // Data.
----------------
I'd go for a constant here as well

These can be class level, probably

================
Comment at: lib/CodeGen/CGCUDANV.cpp:249
@@ +248,3 @@
+        ".cuda_fatbin_wrapper");
+    FatbinWrapper->setAlignment(8);
+
----------------
Comment explaining why

================
Comment at: lib/CodeGen/CGCUDARuntime.h:54
@@ +53,3 @@
+  /// kernel launch stub.
+  virtual void EmitDeviceStub(CodeGenFunction &CGF, FunctionArgList &Args);
+
----------------
I'd move this to the implementation as well, along with EmittedKernels.

Just reading the documentation of this method makes little sense given that it lives in an abstract interface. The code will be easier to untangle if the interface stays completely functionality-free. At this time this won't even add code duplication since we just have a single implementation.

================
Comment at: lib/Driver/Driver.cpp:1235
@@ +1234,3 @@
+
+    std::unique_ptr<Action> HostAction(
+        new CudaHostAction(std::move(Current), CudaDeviceJobActions));
----------------
you can just return new CudaHostAction... here, no?

================
Comment at: lib/Driver/Driver.cpp:1522
@@ -1405,1 +1521,3 @@
 
+#if DISABLED_FOR_NOW
+    // TODO: Cuda compilation has more than one input. Need to figure out how to
----------------
Remove

================
Comment at: lib/Driver/Tools.cpp:2591
@@ +2590,3 @@
+    // Cuda compilation mode may pass more than one file.
+    // Verify that all additional files were derived from the same source.
+    IsCuda = true;
----------------
Can you explain a bit more why/what this means in the comment?

http://reviews.llvm.org/D8463

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/