jhuber6 added inline comments.
================ Comment at: clang/lib/CodeGen/CGCUDARuntime.h:58 + /// Mark the entry as a global variable. + OffloadGlobalEntry = 0x0, + /// Mark the entry as a managed global variable. ---------------- tra wrote: > We're still using the same numeric value for two different kinds of entities. > Considering that it's the third round we're making around this point, I'm > starting to suspect that I may be missing something. > Is there a particular reason kernels and global unmanaged variables have to > have the same 'kind'? > > It's possible that I didn't do a good job explaining my enthusiastic > nitpicking here. > My suggestion to have unified enum for **all** entities we register is based > on a principle of separation of responsibilities. If we want to know what > kind of entry we're dealing with, checking the 'kind' field should be > sufficient. The 'size' field should only indicate the size of the entity. > Having to consider both kind and size to determine what you're dealing with > just muddies things and should not be done unless there's a good reason for > that. E.g. it might be OK if we were short on flag bits. > > Ah, I see the point you're making now. This is yet another thing that OpenMP did that I just copied. I wouldn't have implemented it this way but I figured it would be simpler to keep them similar. I mostly did it this way because I did some initial tests of registering and accessing CUDA globals in OpenMP and it required using the same flags for the kernels and globals. We could change it for CUDA in the future and I could make that change here if it's valuable. Ideally I would like to rewrite how we do all this registration with the structs but breaking the ABI makes it complicated... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123471/new/ https://reviews.llvm.org/D123471 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
