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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
