Page MenuHomePhabricator

[OPENMP]Referencing a static function defined in declare variant is generating an erroneous warning.
AcceptedPublic

Authored by zahiraam on Wed, Jun 2, 8:39 AM.

Details

Summary

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.

Diff Detail

Unit TestsFailed

TimeTest
160 msx64 debian > Clang.AST::ast-dump-openmp-begin-declare-variant_template_3.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -triple x86_64-unknown-unknown -fopenmp -verify -ast-dump /var/lib/buildkite-agent/builds/llvm-project/clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp -x c++| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/AST/ast-dump-openmp-begin-declare-variant_template_3.cpp
120 msx64 windows > Clang.AST::ast-dump-openmp-begin-declare-variant_template_3.cpp
Script: -- : 'RUN: at line 1'; c:\ws\w5\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w5\llvm-project\premerge-checks\build\lib\clang\13.0.0\include -nostdsysteminc -triple x86_64-unknown-unknown -fopenmp -verify -ast-dump C:\ws\w5\llvm-project\premerge-checks\clang\test\AST\ast-dump-openmp-begin-declare-variant_template_3.cpp -x c++| c:\ws\w5\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w5\llvm-project\premerge-checks\clang\test\AST\ast-dump-openmp-begin-declare-variant_template_3.cpp

Event Timeline

zahiraam requested review of this revision.Wed, Jun 2, 8:39 AM
zahiraam created this revision.
zahiraam updated this revision to Diff 349640.Thu, Jun 3, 12:12 PM
zahiraam updated this revision to Diff 350052.Sat, Jun 5, 5:08 AM

Review anyone please? Thanks.

Review anyone please? Thanks.

Add more description to the summary. Would be good to see an analysis, why do we get this error message.

ABataev retitled this revision from Referencing a static function defined in an opnemp clause is generating an erroneous warning. to [OPENMP]Referencing a static function defined in declare variant is generating an erroneous warning..Mon, Jun 7, 4:54 AM
ABataev added inline comments.Mon, Jun 7, 4:55 AM
clang/test/OpenMP/declare_variant.cpp
7

No need for semicolon after braces

zahiraam added a comment.EditedMon, Jun 7, 6:42 AM

Review anyone please? Thanks.

Add more description to the summary. Would be good to see an analysis, why do we get this error message.

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.

zahiraam updated this revision to Diff 350268.Mon, Jun 7, 6:43 AM

Review anyone please? Thanks.

Add more description to the summary. Would be good to see an analysis, why do we get this error message.

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.

ABataev added inline comments.Mon, Jun 7, 7:21 AM
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.

zahiraam updated this revision to Diff 350300.Mon, Jun 7, 8:16 AM
zahiraam added inline comments.
clang/test/OpenMP/declare_variant.cpp
6–8
ABataev added inline comments.Mon, Jun 7, 8:18 AM
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

zahiraam updated this revision to Diff 350388.Mon, Jun 7, 12:41 PM
ABataev added inline comments.Mon, Jun 7, 12:42 PM
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.

I assume the proper fix is to handle declare variants in checkUndefinedButUsed in Sema.cpp. This should fix both PRs, mentioned here

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?

zahiraam added inline comments.Mon, Jun 7, 1:03 PM
clang/test/OpenMP/declare_variant.cpp
15–17

Doing that results into an (expected?) error:
error: use of undeclared identifier 'baz'; did you mean 'bar'?

baz();
^~~
bar
ABataev added inline comments.Mon, Jun 7, 1:06 PM
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();
ABataev added inline comments.Mon, Jun 7, 1:40 PM
clang/lib/Sema/Sema.cpp
814–815 ↗(On Diff #350388)

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.

zahiraam updated this revision to Diff 351129.Thu, Jun 10, 4:27 AM
zahiraam added inline comments.Thu, Jun 10, 4:30 AM
clang/lib/Sema/Sema.cpp
814–815 ↗(On Diff #350388)

@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.

ABataev added inline comments.Thu, Jun 10, 5:27 AM
clang/lib/Sema/Sema.cpp
815 ↗(On Diff #351129)

auto *

824 ↗(On Diff #351129)

This check looks strange to me. What is it for?

zahiraam added inline comments.Thu, Jun 10, 5:35 AM
clang/lib/Sema/Sema.cpp
824 ↗(On Diff #351129)

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.

ABataev added inline comments.Thu, Jun 10, 6:06 AM
clang/lib/Sema/Sema.cpp
824 ↗(On Diff #351129)

No need to check for the name, better to check for something like BaseD->getIdentifier()->isMangledOpenMPVariantName()

zahiraam updated this revision to Diff 351175.Thu, Jun 10, 7:55 AM
zahiraam marked an inline comment as done.
zahiraam added inline comments.
clang/lib/Sema/Sema.cpp
824 ↗(On Diff #351129)

Func is the one that is mangled. Agree?

ABataev added inline comments.Thu, Jun 10, 8:01 AM
clang/lib/Sema/Sema.cpp
817 ↗(On Diff #351175)

const auto *

821–822 ↗(On Diff #351175)

const auto * and cast instead of dyn_cast

zahiraam updated this revision to Diff 351241.Thu, Jun 10, 12:38 PM
ABataev added inline comments.Thu, Jun 10, 12:40 PM
clang/lib/Sema/Sema.cpp
817 ↗(On Diff #351241)

const auto *

clang/test/OpenMP/declare_variant.cpp
5

Add //TODO that this must be fixed, please.

zahiraam added inline comments.Thu, Jun 10, 12:43 PM
clang/test/OpenMP/declare_variant.cpp
5

Yes! Was wondering if it is not better to remove the this part of the test case all together?

zahiraam updated this revision to Diff 351254.Thu, Jun 10, 1:46 PM
zahiraam marked an inline comment as done.
zahiraam added inline comments.
clang/lib/Sema/Sema.cpp
817 ↗(On Diff #351241)

auto* only because the const quallifier is not expected for getTraitInfo().

ABataev accepted this revision.Thu, Jun 10, 1:49 PM

LG, but add an extra description in summary

This revision is now accepted and ready to land.Thu, Jun 10, 1:49 PM
zahiraam edited the summary of this revision. (Show Details)Thu, Jun 10, 2:27 PM

LG, but add an extra description in summary

Thanks for taking time to review.

zahiraam updated this revision to Diff 351272.Thu, Jun 10, 2:48 PM