This is an archive of the discontinued LLVM Phabricator instance.

[Clang][Sema][OpenMP] Sema support for `atomic compare`
ClosedPublic

Authored by tianshilei1992 on Jan 4 2022, 6:09 PM.

Details

Summary

This patch adds the Sema support for atomic compare.

Diff Detail

Event Timeline

tianshilei1992 created this revision.Jan 4 2022, 6:09 PM
tianshilei1992 requested review of this revision.Jan 4 2022, 6:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2022, 6:09 PM
koops added a subscriber: koops.Jan 18 2022, 2:40 AM

sema and parsing tests?

clang/lib/Sema/SemaOpenMP.cpp
10921

static

10928

static

sema and parsing tests?

Tests will be in the patch for code gen.

clang/lib/Sema/SemaOpenMP.cpp
10921

Sure, but I usually prefer not to use static for a function in an anonymous name space.

sema and parsing tests?

Tests will be in the patch for code gen.

sema and parsing tests are different from code gen tests.
The former goes here, e.g., look at _messages tests.

sema and parsing tests?

Tests will be in the patch for code gen.

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.

Fix template instantiation and add one test.
More tests are on the way.

tianshilei1992 marked 2 inline comments as done.Feb 3 2022, 8:00 AM

refine the test

ABataev added inline comments.Feb 3 2022, 10:23 AM
clang/lib/Sema/SemaOpenMP.cpp
11018

body_empty()

11197

QualType

11232

body_empty()

ABataev added inline comments.Feb 3 2022, 10:25 AM
clang/test/OpenMP/atomic_ast_print.cpp
35 ↗(On Diff #405703)

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:

  1. 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.
  2. 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.
  3. 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.

tianshilei1992 added inline comments.Feb 3 2022, 10:28 AM
clang/test/OpenMP/atomic_ast_print.cpp
35 ↗(On Diff #405703)

Yeah, I'll also do that.

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

  1. 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.
  2. 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.
  3. 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.

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.

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

  1. 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.
  2. 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.
  3. 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.

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.

For the 2nd method, -verify doesn't work with -emit-pch. Any way to work around it?

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

  1. 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.
  2. 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.
  3. 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.

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.

For the 2nd method, -verify doesn't work with -emit-pch. Any way to work around it?

Do not use it with emit-pch, just like in the test you modified

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

  1. 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.
  2. 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.
  3. 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.

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.

For the 2nd method, -verify doesn't work with -emit-pch. Any way to work around it?

Do not use it with emit-pch, just like in the test you modified

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.

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

  1. 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.
  2. 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.
  3. 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.

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.

For the 2nd method, -verify doesn't work with -emit-pch. Any way to work around it?

Do not use it with emit-pch, just like in the test you modified

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

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

  1. 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.
  2. 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.
  3. 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.

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.

For the 2nd method, -verify doesn't work with -emit-pch. Any way to work around it?

Do not use it with emit-pch, just like in the test you modified

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

Oh yeah, that is a good way. Thanks.

tianshilei1992 added inline comments.Feb 3 2022, 11:31 AM
clang/test/OpenMP/atomic_ast_print.cpp
10 ↗(On Diff #405725)

@ABataev But still in this path, we cannot test -emit-pch because we will emit atomic compare is not supported for now anyway, and it cannot be worked around. I'll add it in the codegen patch or in a following patch of codegen.

tianshilei1992 added inline comments.Feb 3 2022, 11:33 AM
clang/test/OpenMP/atomic_ast_print.cpp
964 ↗(On Diff #405725)

I cannot figure out why I have to add this line and it's not needed before.

jdoerfert added inline comments.Feb 3 2022, 11:41 AM
clang/test/OpenMP/atomic_ast_print.cpp
964 ↗(On Diff #405725)

it's caused by the error.

tianshilei1992 marked an inline comment as done.Feb 3 2022, 12:11 PM
tianshilei1992 added inline comments.
clang/test/OpenMP/atomic_ast_print.cpp
964 ↗(On Diff #405725)

Aha, I see. Thanks.

tianshilei1992 marked an inline comment as done.

add more tests

tianshilei1992 marked 3 inline comments as done.Feb 3 2022, 4:43 PM
tianshilei1992 added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
11197

Do you mean by not using auto?

add final test and fix comments

tianshilei1992 marked an inline comment as done.Feb 3 2022, 5:18 PM
tianshilei1992 edited the summary of this revision. (Show Details)Feb 3 2022, 5:26 PM
ABataev added inline comments.Feb 4 2022, 6:15 AM
clang/test/OpenMP/atomic_ast_print.cpp
10 ↗(On Diff #405725)

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?

tianshilei1992 marked an inline comment as done.Feb 4 2022, 6:28 AM
tianshilei1992 added inline comments.
clang/test/OpenMP/atomic_ast_print.cpp
10 ↗(On Diff #405725)

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.

tianshilei1992 marked an inline comment as done.Feb 4 2022, 6:29 AM
ABataev added inline comments.Feb 4 2022, 6:30 AM
clang/test/OpenMP/atomic_ast_print.cpp
10 ↗(On Diff #405725)

Ah, I see. Add a TODO to the tests to add pch testing.

add TODO for PCH test

tianshilei1992 marked an inline comment as done.Feb 4 2022, 7:37 AM
ABataev added inline comments.Feb 4 2022, 7:39 AM
clang/test/OpenMP/atomic_ast_print.cpp
40 ↗(On Diff #405967)

Try to add -fsyntax-only in RUN to avoid error message in the test

tianshilei1992 marked an inline comment as done.Feb 4 2022, 7:59 AM
tianshilei1992 added inline comments.
clang/test/OpenMP/atomic_ast_print.cpp
40 ↗(On Diff #405967)

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.
tianshilei1992 marked an inline comment as done.Feb 4 2022, 7:59 AM
ABataev added inline comments.Feb 4 2022, 8:05 AM
clang/test/OpenMP/atomic_ast_print.cpp
40 ↗(On Diff #405967)

Ah, yes. Can you move the error message to codegen from Sema to avoid all these problems?

tianshilei1992 marked an inline comment as done.Feb 4 2022, 8:10 AM
tianshilei1992 added inline comments.
clang/test/OpenMP/atomic_ast_print.cpp
40 ↗(On Diff #405967)

Do we support error message in code gen? I didn't see similar code in clang/lib/CodeGen/CGStmtOpenMP.cpp.

ABataev added inline comments.Feb 4 2022, 8:12 AM
clang/test/OpenMP/atomic_ast_print.cpp
40 ↗(On Diff #405967)

Check lib/CodeGen/CGOpenMPRuntime.cpp, getDiagnostics().Report() calls.

tianshilei1992 marked an inline comment as done.

move error to code gen and add missing tests

tianshilei1992 marked an inline comment as done.Feb 4 2022, 8:45 AM
This revision is now accepted and ready to land.Feb 4 2022, 8:47 AM
This revision was landed with ongoing or failed builds.Feb 4 2022, 9:31 AM
This revision was automatically updated to reflect the committed changes.
cmtice added a subscriber: cmtice.Feb 4 2022, 10:07 AM

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 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?

Yeah, I noticed that. It's been fixed now. Thanks.

I pushed a fix about half an hour ago. It should fix the issue. Thanks.

I pushed a fix about half an hour ago. It should fix the issue. Thanks.

I doubt it is fixed. Also, please reference the phab review in a fix to make it easier to find it.

I pushed a fix about half an hour ago. It should fix the issue. Thanks.

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.

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.

Thanks! It has been fixed.