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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to