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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
