[PATCH] D102801: [CUDA][HIP] Fix device variables used by host

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D102801#2771664 <https://reviews.llvm.org/D102801#2771664>, @yaxunl wrote:

> In the updated patch I have a simpler solution which is easier to explain to the users. Basically we classify variables by how they are emitted: device side only, host side only, both sides as different entities (e.g. default constexpr var), and both sides as unified entity (e.g. managed var). For variables emitted on both sides as separate entities, we have limited knowledge and we limit what we can do for them. I think users should understand the compiler's limitation in such cases. And they can easily workaround that by making the variable explicitly device variable.

This is really nice.

Let me test it internally and see if anything breaks.



================
Comment at: clang/include/clang/Sema/Sema.h:12066
 
+  enum CUDAVariableTarget {
+    CVT_Device,  /// Device only
----------------
Wasn't there another kind, where the variable is emitted on the host with device-side shadow? I vaguely recall it had something to do with textures.


================
Comment at: clang/include/clang/Sema/Sema.h:12067
+  enum CUDAVariableTarget {
+    CVT_Device,  /// Device only
+    CVT_Host,    /// Host only
----------------
I think we should mention the host-side shadows, too.


================
Comment at: clang/lib/Sema/SemaCUDA.cpp:148-149
+    return CVT_Unified;
+  if (hasImplicitAttr<CUDAConstantAttr>(Var))
+    return CVT_Both;
+  if (Var->hasAttr<CUDADeviceAttr>() || Var->hasAttr<CUDAConstantAttr>() ||
----------------
I'm still not a fan of relying on a implicit __constant__.
Can we change it to more direct `is-a-constexpr && !has-explicit-device-side-attr` ?
We may eventually consider relaxing this to `can-be-const-evaluated` and allow const vars with known values.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102801/new/

https://reviews.llvm.org/D102801