[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added a comment.

Fix is here D80829  .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387



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

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added a comment.

In D74387#2063423 , @ABataev wrote:

> Seems to me, this patch crashes 
> `llvm-project/openmp/libomptarget/test/mapping/declare_mapper_api.cpp`.


It seems this patch caused asking size of dependent type, AST context doesn't 
seem expecting it. I'll provide follow up fix shortly. Sorry for inconvenience.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387



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

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Seems to me, this patch crashes 
`llvm-project/openmp/libomptarget/test/mapping/declare_mapper_api.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387



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

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Alexey Bader via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcf6cc662: [OpenMP][SYCL] Improve diagnosing of 
unsupported types usage (authored by Fznamznon, committed by bader).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaSYCL.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Headers/nvptx_device_math_sin.c
  clang/test/Headers/nvptx_device_math_sin.cpp
  clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp
  clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
  clang/test/SemaSYCL/float128.cpp

Index: clang/test/SemaSYCL/float128.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/float128.cpp
@@ -0,0 +1,96 @@
+// RUN: %clang_cc1 -triple spir64 -fsycl -fsycl-is-device -verify -fsyntax-only %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsycl -fsycl-is-device -fsyntax-only %s
+
+typedef __float128 BIGTY;
+
+template 
+class Z {
+public:
+  // expected-note@+1 {{'field' defined here}}
+  T field;
+  // expected-note@+1 2{{'field1' defined here}}
+  __float128 field1;
+  using BIGTYPE = __float128;
+  // expected-note@+1 {{'bigfield' defined here}}
+  BIGTYPE bigfield;
+};
+
+void host_ok(void) {
+  __float128 A;
+  int B = sizeof(__float128);
+  Z<__float128> C;
+  C.field1 = A;
+}
+
+void usage() {
+  // expected-note@+1 3{{'A' defined here}}
+  __float128 A;
+  Z<__float128> C;
+  // expected-error@+2 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  // expected-error@+1 {{'field1' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  C.field1 = A;
+  // expected-error@+1 {{'bigfield' requires 128 bit size 'Z::BIGTYPE' (aka '__float128') type support, but device 'spir64' does not support it}}
+  C.bigfield += 1.0;
+
+  // expected-error@+1 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  auto foo1 = [=]() {
+__float128 AA;
+// expected-note@+2 {{'BB' defined here}}
+// expected-error@+1 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+auto BB = A;
+// expected-error@+1 {{'BB' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+BB += 1;
+  };
+
+  // expected-note@+1 {{called by 'usage'}}
+  foo1();
+}
+
+template 
+void foo2(){};
+
+// expected-note@+3 {{'P' defined here}}
+// expected-error@+2 {{'P' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+// expected-note@+1 2{{'foo' defined here}}
+__float128 foo(__float128 P) { return P; }
+
+template 
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+  // expected-note@+1 5{{called by 'kernel}}
+  kernelFunc();
+}
+
+int main() {
+  // expected-note@+1 {{'CapturedToDevice' defined here}}
+  __float128 CapturedToDevice = 1;
+  host_ok();
+  kernel([=]() {
+decltype(CapturedToDevice) D;
+// expected-error@+1 {{'CapturedToDevice' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+auto C = CapturedToDevice;
+Z<__float128> S;
+// expected-error@+1 {{'field1' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+S.field1 += 1;
+// expected-error@+1 {{'field' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+S.field = 1;
+  });
+
+  kernel([=]() {
+// expected-note@+1 2{{called by 'operator()'}}
+usage();
+// expected-note@+1 {{'' defined here}}
+BIGTY ;
+// expected-note@+3 {{called by 'operator()'}}
+// expected-error@+2 2{{'foo' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+// expected-error@+1 {{'' requires 128 bit size 'BIGTY' (aka '__float128') type support, but device 'spir64' does not support it}}
+auto A = foo();
+  });
+
+  kernel([=]() {
+Z<__float128> S;
+foo2<__float128>();
+auto A = sizeof(CapturedToDevice);
+  });
+
+  return 0;
+}
Index: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
===
--- clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
+++ clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
@@ -7,18 +7,23 @@
 struct T {
   char a;
 #ifndef _ARCH_PPC
+  // expected-note@+1 {{'f' defined here}}
   __float128 f;
 #else
+  // expected-note@+1 {{'f' defined here}}
   long double f;
 #endif
   char c;
   T() : a(12), f(15) {}
 #ifndef _ARCH_P

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon updated this revision to Diff 266942.
Fznamznon marked an inline comment as done and an inline comment as not done.
Fznamznon added a comment.

Included test cases from Johannes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaSYCL.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Headers/nvptx_device_math_sin.c
  clang/test/Headers/nvptx_device_math_sin.cpp
  clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp
  clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
  clang/test/SemaSYCL/float128.cpp

Index: clang/test/SemaSYCL/float128.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/float128.cpp
@@ -0,0 +1,96 @@
+// RUN: %clang_cc1 -triple spir64 -fsycl -fsycl-is-device -verify -fsyntax-only %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsycl -fsycl-is-device -fsyntax-only %s
+
+typedef __float128 BIGTY;
+
+template 
+class Z {
+public:
+  // expected-note@+1 {{'field' defined here}}
+  T field;
+  // expected-note@+1 2{{'field1' defined here}}
+  __float128 field1;
+  using BIGTYPE = __float128;
+  // expected-note@+1 {{'bigfield' defined here}}
+  BIGTYPE bigfield;
+};
+
+void host_ok(void) {
+  __float128 A;
+  int B = sizeof(__float128);
+  Z<__float128> C;
+  C.field1 = A;
+}
+
+void usage() {
+  // expected-note@+1 3{{'A' defined here}}
+  __float128 A;
+  Z<__float128> C;
+  // expected-error@+2 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  // expected-error@+1 {{'field1' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  C.field1 = A;
+  // expected-error@+1 {{'bigfield' requires 128 bit size 'Z::BIGTYPE' (aka '__float128') type support, but device 'spir64' does not support it}}
+  C.bigfield += 1.0;
+
+  // expected-error@+1 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  auto foo1 = [=]() {
+__float128 AA;
+// expected-note@+2 {{'BB' defined here}}
+// expected-error@+1 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+auto BB = A;
+// expected-error@+1 {{'BB' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+BB += 1;
+  };
+
+  // expected-note@+1 {{called by 'usage'}}
+  foo1();
+}
+
+template 
+void foo2(){};
+
+// expected-note@+3 {{'P' defined here}}
+// expected-error@+2 {{'P' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+// expected-note@+1 2{{'foo' defined here}}
+__float128 foo(__float128 P) { return P; }
+
+template 
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+  // expected-note@+1 5{{called by 'kernel}}
+  kernelFunc();
+}
+
+int main() {
+  // expected-note@+1 {{'CapturedToDevice' defined here}}
+  __float128 CapturedToDevice = 1;
+  host_ok();
+  kernel([=]() {
+decltype(CapturedToDevice) D;
+// expected-error@+1 {{'CapturedToDevice' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+auto C = CapturedToDevice;
+Z<__float128> S;
+// expected-error@+1 {{'field1' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+S.field1 += 1;
+// expected-error@+1 {{'field' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+S.field = 1;
+  });
+
+  kernel([=]() {
+// expected-note@+1 2{{called by 'operator()'}}
+usage();
+// expected-note@+1 {{'' defined here}}
+BIGTY ;
+// expected-note@+3 {{called by 'operator()'}}
+// expected-error@+2 2{{'foo' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+// expected-error@+1 {{'' requires 128 bit size 'BIGTY' (aka '__float128') type support, but device 'spir64' does not support it}}
+auto A = foo();
+  });
+
+  kernel([=]() {
+Z<__float128> S;
+foo2<__float128>();
+auto A = sizeof(CapturedToDevice);
+  });
+
+  return 0;
+}
Index: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
===
--- clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
+++ clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
@@ -7,18 +7,23 @@
 struct T {
   char a;
 #ifndef _ARCH_PPC
+  // expected-note@+1 {{'f' defined here}}
   __float128 f;
 #else
+  // expected-note@+1 {{'f' defined here}}
   long double f;
 #endif
   char c;
   T() : a(12), f(15) {}
 #ifndef _ARCH_PPC
-// expected-error@+4

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Can you include these:

  long double qa, qb;
  decltype(qa + qb) qc;
  double qd[sizeof(-(-(qc * 2)))];


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387



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

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

> This change even doesn't break my local check-clang LIT tests run, but I'm 
> not really sure that such change is in scope of this patch, because 
> DiagnoseUseOfDecl contains a lot of other diagnostics as well.

Fair. Let's do the following. We go with this as it is a clear improvement and 
almost complete. If you could provide a follow up to call `DiagnoseUseOfDecl`, 
or maybe just to the part we actually need, for member initialization, we can 
ask Clang folks to take a look. I think there is a reasonable way forward.

LGTM. Thanks a lot for implementing this in a generic and sharable way!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387



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

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon marked 2 inline comments as done.
Fznamznon added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1727
+
+  QualType Ty = D->getType();
+  auto CheckType = [&](QualType Ty) {

jdoerfert wrote:
> Nit: Move below `CheckType` to avoid shadowing and confusion with the arg 
> there. 
Done, thanks



Comment at: clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp:21
   char c;
   T() : a(12), f(15) {}
   T &operator+(T &b) { f += b.a; return *this;}

jdoerfert wrote:
> Why is this not diagnosed? I mean we cannot assign 15 on the device, can we? 
> Or does it work because it is a constant (and we basically just memcpy 
> something)?
> 
> If it's the latter, do we have a test in the negative version that makes sure 
> `T(int i) : a(i), f(i) {}` is flagged?
Unfortunately, nor this case neither `T(int i) : a(i), f(i) {}` is not 
diagnosed. This happens because `DiagnoseUseOfDecl` call seems missing for 
member initializers, not because there is memcpy. So, for example, such case is 
diagnosed:
```
struct B {
  __float128 a;
};
#pragma omp declare target
void foo() {
  B var = {1}; // error: 'a' requires 128 bit size '__float128' type support, 
but device 'nvptx64-unknown-unknown' does not support it
}
```
`DiagnoseUseOfDecl` function is called in so many cases and I guess it is meant 
to be called on each usage of each declaration, that is why I think the correct 
fix is add call to `DiagnoseUseOfDecl` somewhere near building of member 
initializers . This change even doesn't break my local `check-clang` LIT tests 
run, but I'm not really sure that such change is in scope of this patch, 
because `DiagnoseUseOfDecl` contains a lot of other diagnostics as well.



Comment at: clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp:81
-// CHECK: define weak void 
@__omp_offloading_{{.+}}foo{{.+}}_l75([[BIGTYPE:.+]]*
-// CHECK: store [[BIGTYPE]] 
{{0xL3FFF|0xM3FF0}}, 
[[BIGTYPE]]* %

jdoerfert wrote:
> Just checking, we verify in the other test this would result in an error, 
> right?
Yes, I added such test case in `nvptx_unsupported_type_messages.cpp` .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387



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

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon updated this revision to Diff 266877.
Fznamznon marked 2 inline comments as done.
Fznamznon added a comment.

Applied comments from Johannes.
Fixed failing tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaSYCL.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Headers/nvptx_device_math_sin.c
  clang/test/Headers/nvptx_device_math_sin.cpp
  clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp
  clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
  clang/test/SemaSYCL/float128.cpp

Index: clang/test/SemaSYCL/float128.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/float128.cpp
@@ -0,0 +1,96 @@
+// RUN: %clang_cc1 -triple spir64 -fsycl -fsycl-is-device -verify -fsyntax-only %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsycl -fsycl-is-device -fsyntax-only %s
+
+typedef __float128 BIGTY;
+
+template 
+class Z {
+public:
+  // expected-note@+1 {{'field' defined here}}
+  T field;
+  // expected-note@+1 2{{'field1' defined here}}
+  __float128 field1;
+  using BIGTYPE = __float128;
+  // expected-note@+1 {{'bigfield' defined here}}
+  BIGTYPE bigfield;
+};
+
+void host_ok(void) {
+  __float128 A;
+  int B = sizeof(__float128);
+  Z<__float128> C;
+  C.field1 = A;
+}
+
+void usage() {
+  // expected-note@+1 3{{'A' defined here}}
+  __float128 A;
+  Z<__float128> C;
+  // expected-error@+2 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  // expected-error@+1 {{'field1' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  C.field1 = A;
+  // expected-error@+1 {{'bigfield' requires 128 bit size 'Z::BIGTYPE' (aka '__float128') type support, but device 'spir64' does not support it}}
+  C.bigfield += 1.0;
+
+  // expected-error@+1 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  auto foo1 = [=]() {
+__float128 AA;
+// expected-note@+2 {{'BB' defined here}}
+// expected-error@+1 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+auto BB = A;
+// expected-error@+1 {{'BB' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+BB += 1;
+  };
+
+  // expected-note@+1 {{called by 'usage'}}
+  foo1();
+}
+
+template 
+void foo2(){};
+
+// expected-note@+3 {{'P' defined here}}
+// expected-error@+2 {{'P' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+// expected-note@+1 2{{'foo' defined here}}
+__float128 foo(__float128 P) { return P; }
+
+template 
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+  // expected-note@+1 5{{called by 'kernel}}
+  kernelFunc();
+}
+
+int main() {
+  // expected-note@+1 {{'CapturedToDevice' defined here}}
+  __float128 CapturedToDevice = 1;
+  host_ok();
+  kernel([=]() {
+decltype(CapturedToDevice) D;
+// expected-error@+1 {{'CapturedToDevice' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+auto C = CapturedToDevice;
+Z<__float128> S;
+// expected-error@+1 {{'field1' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+S.field1 += 1;
+// expected-error@+1 {{'field' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+S.field = 1;
+  });
+
+  kernel([=]() {
+// expected-note@+1 2{{called by 'operator()'}}
+usage();
+// expected-note@+1 {{'' defined here}}
+BIGTY ;
+// expected-note@+3 {{called by 'operator()'}}
+// expected-error@+2 2{{'foo' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+// expected-error@+1 {{'' requires 128 bit size 'BIGTY' (aka '__float128') type support, but device 'spir64' does not support it}}
+auto A = foo();
+  });
+
+  kernel([=]() {
+Z<__float128> S;
+foo2<__float128>();
+auto A = sizeof(CapturedToDevice);
+  });
+
+  return 0;
+}
Index: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
===
--- clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
+++ clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
@@ -7,18 +7,23 @@
 struct T {
   char a;
 #ifndef _ARCH_PPC
+  // expected-note@+1 {{'f' defined here}}
   __float128 f;
 #else
+  // expected-note@+1 {{'f' defined here}}
   long double f;
 #endif
   char c;
   T() : a(12), f(15) {}
 #ifndef _ARCH_PPC
-// expected-error@+4 {{host requires

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D74387#2053742 , @Fznamznon wrote:

> Re-implemented diagnostic itself, now only usages of declarations
>  with unsupported types are diagnosed.
>  Generalized approach between OpenMP and SYCL.


Great, thanks a lot!

In D74387#2054337 , @Fznamznon wrote:

> The tests are failing because calling function with unsupported type in 
> arguments/return value is diagnosed as well, i.e. :
>
>   double math(float f, double d, long double ld) { ... } // `ld` is not used 
> inside the `math` function
>   #pragma omp target map(r)
> { r += math(f, d, ld); } // error: 'math' requires 128 bit size 'long 
> double' type support, but device 'nvptx64-nvidia-cuda' does not support it
>
>
> Should we diagnose calls to such functions even if those arguments/return 
> value aren't used?


Yes, please! The test case (which I added) is broken and would result in a 
crash when you actually ask for PTX and not IR: https://godbolt.org/z/vL5Biw
This is exactly what we need to diagnose :)

---

I think the code looks good and this looks like a really nice way to fix this 
properly.

I inlined some questions. We might need to add some test coverage (if we 
haven't already), e.g., for the memcpy case. For example in OpenMP an object 
`X` with such types should be ok in a `map(tofrom:X)` clause.




Comment at: clang/lib/Sema/Sema.cpp:1727
+
+  QualType Ty = D->getType();
+  auto CheckType = [&](QualType Ty) {

Nit: Move below `CheckType` to avoid shadowing and confusion with the arg 
there. 



Comment at: clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp:21
   char c;
   T() : a(12), f(15) {}
   T &operator+(T &b) { f += b.a; return *this;}

Why is this not diagnosed? I mean we cannot assign 15 on the device, can we? Or 
does it work because it is a constant (and we basically just memcpy something)?

If it's the latter, do we have a test in the negative version that makes sure 
`T(int i) : a(i), f(i) {}` is flagged?



Comment at: clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp:81
-// CHECK: define weak void 
@__omp_offloading_{{.+}}foo{{.+}}_l75([[BIGTYPE:.+]]*
-// CHECK: store [[BIGTYPE]] 
{{0xL3FFF|0xM3FF0}}, 
[[BIGTYPE]]* %

Just checking, we verify in the other test this would result in an error, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387



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

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added a comment.

The tests are failing because calling function with unsupported type in 
arguments/return value is diagnosed as well, i.e. :

  double math(float f, double d, long double ld) { ... } // `ld` is not used 
inside the `math` function
  #pragma omp target map(r)
{ r += math(f, d, ld); } // error: 'math' requires 128 bit size 'long 
double' type support, but device 'nvptx64-nvidia-cuda' does not support it

Should we diagnose calls to such functions even if those arguments/return value 
aren't used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387



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

[PATCH] D74387: [OpenMP][SYCL] Improve diagnosing of unsupported types usage

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon updated this revision to Diff 266066.
Fznamznon added a comment.
Herald added a subscriber: sstefan1.

Re-implemented diagnostic itself, now only usages of declarations
with unsupported types are diagnosed.
Generalized approach between OpenMP and SYCL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaSYCL.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp
  clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
  clang/test/SemaSYCL/float128.cpp

Index: clang/test/SemaSYCL/float128.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/float128.cpp
@@ -0,0 +1,96 @@
+// RUN: %clang_cc1 -triple spir64 -fsycl -fsycl-is-device -verify -fsyntax-only %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsycl -fsycl-is-device -fsyntax-only %s
+
+typedef __float128 BIGTY;
+
+template 
+class Z {
+public:
+  // expected-note@+1 {{'field' defined here}}
+  T field;
+  // expected-note@+1 2{{'field1' defined here}}
+  __float128 field1;
+  using BIGTYPE = __float128;
+  // expected-note@+1 {{'bigfield' defined here}}
+  BIGTYPE bigfield;
+};
+
+void host_ok(void) {
+  __float128 A;
+  int B = sizeof(__float128);
+  Z<__float128> C;
+  C.field1 = A;
+}
+
+void usage() {
+  // expected-note@+1 3{{'A' defined here}}
+  __float128 A;
+  Z<__float128> C;
+  // expected-error@+2 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  // expected-error@+1 {{'field1' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  C.field1 = A;
+  // expected-error@+1 {{'bigfield' requires 128 bit size 'Z::BIGTYPE' (aka '__float128') type support, but device 'spir64' does not support it}}
+  C.bigfield += 1.0;
+
+  // expected-error@+1 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+  auto foo1 = [=]() {
+__float128 AA;
+// expected-note@+2 {{'BB' defined here}}
+// expected-error@+1 {{'A' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+auto BB = A;
+// expected-error@+1 {{'BB' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+BB += 1;
+  };
+
+  // expected-note@+1 {{called by 'usage'}}
+  foo1();
+}
+
+template 
+void foo2(){};
+
+// expected-note@+3 {{'P' defined here}}
+// expected-error@+2 {{'P' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+// expected-note@+1 2{{'foo' defined here}}
+__float128 foo(__float128 P) { return P; }
+
+template 
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+  // expected-note@+1 5{{called by 'kernel}}
+  kernelFunc();
+}
+
+int main() {
+  // expected-note@+1 {{'CapturedToDevice' defined here}}
+  __float128 CapturedToDevice = 1;
+  host_ok();
+  kernel([=]() {
+decltype(CapturedToDevice) D;
+// expected-error@+1 {{'CapturedToDevice' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+auto C = CapturedToDevice;
+Z<__float128> S;
+// expected-error@+1 {{'field1' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+S.field1 += 1;
+// expected-error@+1 {{'field' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+S.field = 1;
+  });
+
+  kernel([=]() {
+// expected-note@+1 2{{called by 'operator()'}}
+usage();
+// expected-note@+1 {{'' defined here}}
+BIGTY ;
+// expected-note@+3 {{called by 'operator()'}}
+// expected-error@+2 2{{'foo' requires 128 bit size '__float128' type support, but device 'spir64' does not support it}}
+// expected-error@+1 {{'' requires 128 bit size 'BIGTY' (aka '__float128') type support, but device 'spir64' does not support it}}
+auto A = foo();
+  });
+
+  kernel([=]() {
+Z<__float128> S;
+foo2<__float128>();
+auto A = sizeof(CapturedToDevice);
+  });
+
+  return 0;
+}
Index: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
===
--- clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
+++ clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
@@ -7,18 +7,23 @@
 struct T {
   char a;
 #ifndef _ARCH_PPC
+  // expected-note@+1 {{'f' defined here}}
   __float128 f;
 #else
+  // expected-note@+1 {{'f' defined here}}
   long double f;
 #endif
   char c;
   T() : a(12), f(15) {}
 #ifndef _ARCH_PPC
-// expected-error@+4 {{host requires 1