[PATCH] D102801: [CUDA][HIP] Fix implicit constant variable

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

This patch does not appear to fix the second regression introduced by the 
D102237 .

Trying to compile the following code triggers an assertion in CGExpr.cpp:

  class a {
  public:
a(char *);
  };
  void b() {
[](char *c) {
  static a d(c);
  d;
};
  }

With assertions disabled it eventually leads to a different error: 
`Module has a nontrivial global ctor, which NVPTX does not support.`
https://godbolt.org/z/sYE1dKr1W


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

[PATCH] D102801: [CUDA][HIP] Fix implicit constant variable

[Artem Belevich via Phabricator via cfe-commits]

tra added a reviewer: rsmith.
tra added a subscriber: rsmith.
tra added a comment.

Tentative LGTM as we need it to fix the regression soon.

Summoning @rsmith for the 'big picture' opinion. 
While the patch may fix this particular regression, I wonder if there's a 
better way to deal with this. We're growing a bit too many nuances that would 
be hard to explain and may cause more corner cases to appear.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2386
+  };
+  if (!HasImplicitConstantAttr(V))
+DeferredDeclsToEmit.push_back(V);

IIUIC, The idea here is that we do not want to emit `constexpr int foo;` on 
device, even if we happen to ODR-use it there.
And the way we detect this is by checking for implicit `__constant__` we happen 
to add to constexpr variables.

I think this may be relying on the implementation details too much. It also 
makes compiler's behavior somewhat surprising -- we would potentially emit 
other variables that do not get any device attributes attribute, but would not 
emit the variables with implicit `__constant__`, which is a device attribute.

I'm not sure if we have any good options here. This may be an acceptable 
compromise, but I wonder if there's a better way to deal with this.

That said, this patch is OK to fix the regression we have now, but we may need 
to revisit this.




Comment at: clang/test/CodeGenCUDA/host-used-device-var.cu:103-131
+// Check implicit constant variable ODR-used by host code is not emitted.
+// DEV-NEG-NOT: _ZN16TestConstexprVar1oE
+namespace TestConstexprVar {
+char o;
+class ou {
+public:
+  ou(char) { __builtin_strlen(); }

This definitely needs some comments. Otherwise this is nearly incomprehensible 
and it's impossible to tell what's going on.


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

[PATCH] D102801: [CUDA][HIP] Fix implicit constant variable

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl created this revision.
yaxunl added a reviewer: tra.
Herald added a reviewer: aaron.ballman.
yaxunl requested review of this revision.

constexpr variables are implicit constant variables in device compilation.
Not all constexpr variables are valid to be emitted on device side, therefore
we should not force emit implicit constant variables even if they are
ODR-used by host code.

This fixes the regression caused by https://reviews.llvm.org/D102237


https://reviews.llvm.org/D102801

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaCUDA.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-constant-var.cu
  clang/test/CodeGenCUDA/host-used-device-var.cu

Index: clang/test/CodeGenCUDA/host-used-device-var.cu
===
--- clang/test/CodeGenCUDA/host-used-device-var.cu
+++ clang/test/CodeGenCUDA/host-used-device-var.cu
@@ -66,8 +66,22 @@
 template 
 __device__ func_t p_add_func = add_func;
 
+// Check non-constant constexpr variables ODR-used by host code only is not emitted.
+// DEV-NEG-NOT: constexpr_var1a
+// DEV-NEG-NOT: constexpr_var1b
+constexpr int constexpr_var1a = 1;
+inline constexpr int constexpr_var1b = 1;
+
+// Check constant constexpr variables ODR-used by host code only.
+// Non-inline constexpr variable has internal linkage, therefore it is not accessible by host and not kept.
+// Inline constexpr variable has linkonce_ord linkage, therefore it can be accessed by host and kept.
+// DEV-NEG-NOT: constexpr_var2a
+// DEV-DAG: @constexpr_var2b = linkonce_odr addrspace(4) externally_initialized constant i32 2
+__constant__ constexpr int constexpr_var2a = 2;
+inline __constant__ constexpr int constexpr_var2b = 2;
+
 void use(func_t p);
-void use(int *p);
+void use(const int *p);
 
 void fun1() {
   use();
@@ -76,20 +90,58 @@
   use(_var);
   use(_var);
   use(p_add_func);
+  use(_var1a);
+  use(_var1b);
+  use(_var2a);
+  use(_var2b);
 }
 
 __global__ void kern1(int **x) {
   *x = 
 }
 
+// Check implicit constant variable ODR-used by host code is not emitted.
+// DEV-NEG-NOT: _ZN16TestConstexprVar1oE
+namespace TestConstexprVar {
+char o;
+class ou {
+public:
+  ou(char) { __builtin_strlen(); }
+};
+template < typename ao > struct aw { static constexpr ao c; };
+class x {
+protected:
+  typedef ou (*y)(const x *);
+  constexpr x(y ag) : ah(ag) {}
+  template < bool * > struct ak;
+  template < typename > struct al {
+static bool am;
+static ak<  > an;
+  };
+  template < typename ao > static x ap() { (void)aw< ao >::c; return x(nullptr); }
+  y ah;
+};
+template < typename ao > bool x::al< ao >::am(< ao >);
+class ar : x {
+public:
+  constexpr ar() : x(as) {}
+  static ou as(const x *) { return 0; }
+  al< ar > av;
+};
+}
+
 // Check the exact list of variables to ensure @_ZL2u4 is not among them.
-// DEV: @llvm.compiler.used = {{[^@]*}} @_Z10p_add_funcIiE {{[^@]*}} @_ZL2u3 {{[^@]*}} @inline_var {{[^@]*}} @u1 {{[^@]*}} @u2 {{[^@]*}} @u5
+// DEV: @llvm.compiler.used = {{[^@]*}} @_Z10p_add_funcIiE {{[^@]*}} @_ZL2u3 {{[^@]*}} @constexpr_var2b {{[^@]*}} @inline_var {{[^@]*}} @u1 {{[^@]*}} @u2 {{[^@]*}} @u5
 
 // HOST-DAG: hipRegisterVar{{.*}}@u1
 // HOST-DAG: hipRegisterVar{{.*}}@u2
 // HOST-DAG: hipRegisterVar{{.*}}@_ZL2u3
+// HOST-DAG: hipRegisterVar{{.*}}@constexpr_var2b
 // HOST-DAG: hipRegisterVar{{.*}}@u5
 // HOST-DAG: hipRegisterVar{{.*}}@inline_var
 // HOST-DAG: hipRegisterVar{{.*}}@_Z10p_add_funcIiE
 // HOST-NEG-NOT: hipRegisterVar{{.*}}@ext_var
 // HOST-NEG-NOT: hipRegisterVar{{.*}}@_ZL2u4
+// HOST-NEG-NOT: hipRegisterVar{{.*}}@constexpr_var1a
+// HOST-NEG-NOT: hipRegisterVar{{.*}}@constexpr_var1b
+// HOST-NEG-NOT: hipRegisterVar{{.*}}@constexpr_var2a
Index: clang/test/AST/ast-dump-constant-var.cu
===
--- /dev/null
+++ clang/test/AST/ast-dump-constant-var.cu
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -std=c++14 -ast-dump -x hip %s | FileCheck -check-prefixes=CHECK,HOST %s
+// RUN: %clang_cc1 -std=c++14 -ast-dump -fcuda-is-device -x hip %s | FileCheck -check-prefixes=CHECK,DEV %s
+
+#include "Inputs/cuda.h"
+
+// CHECK-LABEL: VarDecl {{.*}} m1 'int'
+// CHECK-NEXT: CUDAConstantAttr {{.*}}cuda.h
+__constant__ int m1;
+
+// CHECK-LABEL: VarDecl {{.*}} m2 'int'
+// CHECK-NEXT: CUDAConstantAttr {{.*}}cuda.h
+// CHECK-NOT: CUDAConstantAttr
+__constant__ __constant__ int m2;
+
+// CHECK-LABEL: VarDecl {{.*}} m3 'const int'
+// HOST-NOT: CUDAConstantAttr
+// DEV-NOT: CUDAConstantAttr {{.*}}cuda.h
+// DEV: CUDAConstantAttr {{.*}}Implicit
+// DEV-NOT: CUDAConstantAttr {{.*}}cuda.h
+constexpr int m3 = 1;
+
+// CHECK-LABEL: VarDecl {{.*}} m3a 'const int'
+// CHECK-NOT: CUDAConstantAttr {{.*}}Implicit
+// CHECK: CUDAConstantAttr {{.*}}cuda.h
+// CHECK-NOT: CUDAConstantAttr {{.*}}Implicit
+constexpr __constant__ int m3a = 2;
+
+// CHECK-LABEL: VarDecl {{.*}} m3b 'const int'
+// CHECK-NOT: CUDAConstantAttr {{.*}}Implicit
+// CHECK: CUDAConstantAttr