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: > jhuber6 wrote: > > tra wrote: > > > jhuber6 wrote: > > > > 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... > > > > 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. > > > > > > OK. So, there is something that requires this magic. If that's something > > > we must have, then it must be mentioned in the comments around the enum. > > > > > > Do you know where I should find the code which needs this? I'm curious > > > what's going on there. > > > I wonder if it just checks for "flags==0" and refuses to deal with > > > unknown flags. > > > To think of it, we probably want to put the enum into a common header > > > which defines the `__tgt_offload_entry`.We would not want OpenMP itself > > > to start using the same bits for something else. > > Sorry, I should be more specific. The OpenMP offloading runtime currently > > uses a size of zero to indicate a kernel function and the flags have a > > different meaning if it's a kernel. For OpenMP, 0 is a kernel, 1 and 2 are > > device ctors / dtors. I'm not sure why they chose this over just another > > flag but it's the current standard. You can see it used like this here > > https://github.com/llvm/llvm-project/blob/main/openmp/libomptarget/src/omptarget.cpp#L147. > > I'm not sure if there's a good way to wrangle these together now that I > > think about it, considering OpenMP already uses `0x1` to represent `link` > > OpenMP variables so this already collides. But treating the flags different > > on the size is at least consistent with what OpenMP does. It makes it a > > little hard to define one enum for it since we use it two different ways, > > I'm not a fan of it but it's what the current ABI uses. > I see. > > Using `size=0` as the coda/data flag which changes interpretation of the > flags sort of makes sense. In that case two different types for the flags > field would be appropriate, with an appropriate comment describing that > `size==0` determines which one is in effect. > Personally I'm find with it landing like this, and if we wanted to improve this later it would probably just go in some greater ABI break for offloading entries. There might be a good reason to change them all at once when we start focusing more on complete interoperability of offloading languages. 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
