This patch adds the Sema support for atomic compare.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Tests will be in the patch for code gen.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
10930 | Sure, but I usually prefer not to use static for a function in an anonymous name space. |
sema and parsing tests are different from code gen tests.
The former goes here, e.g., look at _messages tests.
I understand, but currently we raise error in Sema so there is no way it can pass a test.
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
41 | ast print tests are usually successful, better to add/modify special tests for incorrect cases (the ones ending with _messages.cpp) |
@ABataev @jdoerfert I got a question about writing the test. atomic compare is supported from 5.1. Basically we have three ways to guard those atomic compare code:
- Use macro _OPENMP. This pretty much works with one exception: we don't set the macro for -fopenmp-simd. Then we cannot test -fopenmp-simd.
- Use those omp50-error and omp50-note which are shown in this patch. -fopenmp-simd can be tested now. However, it doesn't work with -emit-pch. The compiler emits errors that unexpected OpenMP clause 'compare' in directive '#pragma omp atomic' for the lower version tests. If we add -verify to the -emit-pch line, then the pch will not be generated.
- Put atomic compare tests in another file.
For now only the 3rd method could work, but I'm not sure if there are ways to work around the limitation in the first two approaches.
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
41 | Yeah, I'll also do that. |
The second or the 3rd approach.
The tests should work with emit pch and include pch options, otherwise precompiled modules are broken. Need to test it too.
Then those tests w/o -fopenmp-version=51, clang emits error "unexpected OpenMP clause 'compare' in directive '#pragma omp atomic'" and exits w/ error code, and the test fails.
You can define your own macro for OpenMP51 tests and guard the new code with #ifdef value
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
926 | I cannot figure out why I have to add this line and it's not needed before. |
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
926 | it's caused by the error. |
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
926 | Aha, I see. Thanks. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
11206 | Do you mean by not using auto? |
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
10 | Why? It should be tested. // RUN: %clang_cc1 -fopenmp -DOMP51 -fopenmp-version=51 -x c++ -std=c++11 -emit-pch -o %t %s // RUN: %clang_cc1 -fopenmp -DOMP51 -fopenmp-version=51 -std=c++11 -include-pch %t -fsyntax-only -verify %s -ast-print | FileCheck %s Could you try this? |
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
10 | I did, but since the first command can emit errors, the test fails. It should be tested. I'll add it back when we don't emit error in Sema. |
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
10 | Ah, I see. Add a TODO to the tests to add pch testing. |
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
42 | Try to add -fsyntax-only in RUN to avoid error message in the test |
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
42 | Adding -fsyntax-only will not print the AST, just the error message. ➜ clang -cc1 -x c++ -fopenmp -fopenmp-version=51 -ast-print atomic.cpp atomic.cpp:12:20: error: atomic compare is not supported for now #pragma omp atomic compare ^ class C { int x; public: int &val() { return this->x; } }; void compare() { C x; C e; C d; #pragma omp atomic compare if (x.val() == e.val()) x.val() = d.val(); } 1 error generated. ➜ clang -cc1 -x c++ -fopenmp -fopenmp-version=51 -ast-print -fsyntax-only atomic.cpp atomic.cpp:12:20: error: atomic compare is not supported for now #pragma omp atomic compare ^ 1 error generated. |
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
42 | Ah, yes. Can you move the error message to codegen from Sema to avoid all these problems? |
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
42 | Do we support error message in code gen? I didn't see similar code in clang/lib/CodeGen/CGStmtOpenMP.cpp. |
clang/test/OpenMP/atomic_ast_print.cpp | ||
---|---|---|
42 | Check lib/CodeGen/CGOpenMPRuntime.cpp, getDiagnostics().Report() calls. |
This commit is causing some of our builds to fail with these errors:
clang/lib/Sema/SemaOpenMP.cpp:11372:9: error: unused variable 'D' [-Werror,-Wunused-variable]
Expr *D = nullptr; ^
clang/lib/Sema/SemaOpenMP.cpp:11373:9: error: unused variable 'CE' [-Werror,-Wunused-variable]
Expr *CE = nullptr; ^
2 errors generated.
Would you please fix this?
This still seems to be breaking sanitizers fast: https://lab.llvm.org/buildbot/#/builders/5/builds/18563
To reproduce these results:
https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild
I doubt it is fixed. Also, please reference the phab review in a fix to make it easier to find it.
It will. The BuildBot link here is still the one for this patch. It is fixed in b8ec430de71766d9a35a6b737c8a789c0c7cf812.
When building/running third_party/llvm/llvm-project/clang/test/OpenMP/atomic_messages.c and third_party/llvm/llvm-project/clang/test/OpenMP/atomic_ast_print.cpp, we get use-of-uninitialized-variable error messages:
SUMMARY: MemorySanitizer: use-of-uninitialized-value third_party/llvm/llvm-project/clang/lib/Sema/SemaOpenMP.cpp:11214:40 in (anonymous namespace)::OpenMPAtomicCompareChecker::checkType((anonymous namespace)::OpenMPAtomicCompareChecker::ErrorInfoTy&) const::$_57::operator()(clang::Expr const*, llvm::omp::OMPAtomicCompareOp, bool) const
Exiting
You should probably look into this.
static