[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]] {{0xL00000000000000003FFF000000000000|0xM3FF00000000000000000000000000000}}, [[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