[PATCH] D95912: [OpenMP] Attribute target diagnostics properly

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Thanks for taking a look!



================
Comment at: clang/lib/Sema/Sema.cpp:1798
+  FunctionDecl *FD =
+      isa<FunctionDecl>(C) ? cast<FunctionDecl>(C) : dyn_cast<FunctionDecl>(D);
   auto CheckType = [&](QualType Ty) {
----------------
Fznamznon wrote:
> So, we assume that lexical context is a function and if it is not, we assume that the value declaration being checked is a function. Can we actually break this assumption?
> For example, will something like this work correctly:
> ```
> struct S {
>     __float128 A = 1;
>     int bar = 2 * A;
> };
> #pragma omp declare target
>   S s;
> #pragma omp end declare target
> ```
This is diagnosed neither with nor without the patch.
I'm not sure we should either because 2 * A is a constant expression.
We can make the example more complex using a constructor but I'd say that is not related to this patch either way.


================
Comment at: clang/test/OpenMP/nvptx_unsupported_type_messages.cpp:42
 #ifndef _ARCH_PPC
-// expected-note at +1 {{'boo' defined here}}
+// expected-error at +2 {{'boo' requires 128 bit size '__float128' type support, but device 'nvptx64-unknown-unknown' does not support it}}
+// expected-note at +1 2{{'boo' defined here}}
----------------
Fznamznon wrote:
> So,  `boo` is diagnosed two times, at the point where it actually used and where it is defined, it looks a bit like a duplication isn't it? I think it makes sense to diagnose only usage point with note pointing to the definition.
It is diagnosed twice, right. I don't think this is a problem of this patch though, see for example line 54 on the left where we emit the boo error 4 times before. Also, take a look at 181 and following in this file. With this patch we reduce the number of times the error is emitted from 3 to 1. It's a bit of a mixed bag but I think this makes it better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95912