When a static function is defined inside an OpenMP clause, this warning “function 'foo' has internal linkage but is not defined”. See https://godbolt.org/z/ajKPc36M7
But this warning shouldn’t be issued because the declared variant function is called.
The warning should be generated only when the function is not in an OpenMP clause, or if the function is an implicit base of the declared variant function. This patch does that.
Details
Diff Detail
Event Timeline
Add more description to the summary. Would be good to see an analysis, why do we get this error message.
clang/test/OpenMP/declare_variant.cpp | ||
---|---|---|
7 | No need for semicolon after braces |
The warning warning: function 'foo' has internal linkage but is not defined
[-Wundefined-internal]
is generated at https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/Sema.cpp#L814
In ActoOnstartFunctionDefinitioninOpenMPDeclareVariantScope there is new Decl that is created here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaOpenMP.cpp#L6685. This Decl is added to the UndefinedButUsed map here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExpr.cpp#L17118
This newly created Decl was marked 'imiplicit' but not marked 'used' as expected here https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/DeclBase.h#L578.
Thanks for the context. Not sure that your fix is correct. I assume we still need to emit this warning if the same code is compiled for the device, since there is no variant for the device. Your fix will suppress this warning completely. Also, we have to emit this warning if we have a variant for the device but use it on the host.
clang/test/OpenMP/declare_variant.cpp | ||
---|---|---|
6–8 | Add a test for: #pragma omp begin declare variant match(device = {kind(device)}) static void bar(){} #pragma omp end declare variant ... int main() { ... bar(); ... } We have to see a warning (or some other diagnostics) since we don't have variant for the host. |
Does this by any chance also fix https://bugs.llvm.org/show_bug.cgi?id=49650 ?
clang/test/OpenMP/declare_variant.cpp | ||
---|---|---|
6–8 | The standard says a base declaration is created, so this is legal and before the file is completely scanned we could not even try to emit a warning. If there is no other variant for the host the linker will complain, but that is as expected. |
clang/test/OpenMP/declare_variant.cpp | ||
---|---|---|
6–8 | Right. But I assume this warning is emitted at the end of the compilation module, when we do the analysis in ActOnEndOfTranslationUnit. Definitely, we can rely on the linker but would be good to reuse the existing framework for the analysis, if possible. |
clang/test/OpenMP/declare_variant.cpp | ||
---|---|---|
6–8 | There is a diagnostic triggered by https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseOpenMP.cpp#L889 |
clang/test/OpenMP/declare_variant.cpp | ||
---|---|---|
13 | It is triggered by the wrong kind. Should be nohost, I assume, or something like this. Check the standard |
I assume the proper fix is to handle declare variants in checkUndefinedButUsed in Sema.cpp. This should fix both PRs, mentioned here
clang/test/OpenMP/declare_variant.cpp | ||
---|---|---|
15–17 | Better to rename this function to baz and call it in main(). This should result in a warning. |
Let's see if this is better.
I have added to the LIT test the code that is in https://bugs.llvm.org/show_bug.cgi?id=49650
As for the one you proposed:
#pragma omp begin declare variant match(device = {kind(device)})
static void bar(){}
#pragma omp end declare variant
...
int main() {
...
bar();
...
}'=
I see that it is failing with the same error message that I found locally, https://godbolt.org/z/WWs9EnGhh
Unless you meant something else?
clang/test/OpenMP/declare_variant.cpp | ||
---|---|---|
15–17 | Doing that results into an (expected?) error: baz(); ^~~ bar |
clang/test/OpenMP/declare_variant.cpp | ||
---|---|---|
15–17 | Could you add a declaration outside of the declare variant? static void baz(); #pragma omp begin declare variant match(device = {kind(nohost)}) static void baz() {} #pragma omp end declare variant ... baz(); |
clang/lib/Sema/Sema.cpp | ||
---|---|---|
815–830 | The check does not look correct. It will affect an existing behavior without OpenMP. The warning should be emitted if either in non-OpenMP mode, or if this is an implicit base of the declare variant function. |
clang/lib/Sema/Sema.cpp | ||
---|---|---|
815–830 | @ABataev This is what I came up with. It didn't fix the issue here https://bugs.llvm.org/show_bug.cgi?id=49650 because there is no implicit base. So not sure if this is quite correct, but wanted to run it by you. Please let me know. |
clang/lib/Sema/Sema.cpp | ||
---|---|---|
825 | Here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaOpenMP.cpp#L6693 the name of base function is mangled. In the attached LIT test "foo" is mangled to "foo$ompvariant{{.*}}". This is to test that FuncName and BaseName have the same "prefix". But may be they can never be different? In which case this test is un-necessary. |
clang/lib/Sema/Sema.cpp | ||
---|---|---|
825 | No need to check for the name, better to check for something like BaseD->getIdentifier()->isMangledOpenMPVariantName() |
clang/lib/Sema/Sema.cpp | ||
---|---|---|
825 | Func is the one that is mangled. Agree? |
clang/test/OpenMP/declare_variant.cpp | ||
---|---|---|
4 | Yes! Was wondering if it is not better to remove the this part of the test case all together? |
clang/lib/Sema/Sema.cpp | ||
---|---|---|
817 | auto* only because the const quallifier is not expected for getTraitInfo(). |
auto *