jdoerfert added a comment. In D74387#2053742 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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]] {{0xL00000000000000003FFF000000000000|0xM3FF00000000000000000000000000000}}, [[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
