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

[tianshilei1992]

Summary

This patch adds the Sema support for atomic compare.

[tianshilei1992]

rebase

[ABataev]

sema and parsing tests?

clang/lib/Sema/SemaOpenMP.cpp

10921

static

10928

static

[tianshilei1992]

In D116637#3284773, @ABataev wrote:

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.

[jdoerfert]

In D116637#3284823, @tianshilei1992 wrote:

In D116637#3284773, @ABataev wrote:

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.

[tianshilei1992]

In D116637#3291608, @jdoerfert wrote:

In D116637#3284823, @tianshilei1992 wrote:

In D116637#3284773, @ABataev wrote:

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.

[tianshilei1992]

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

[tianshilei1992]

refine the test

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

11018

body_empty()

11197

QualType

11232

body_empty()

[ABataev]

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)

[tianshilei1992]

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

clang/test/OpenMP/atomic_ast_print.cpp

35 ↗

(On Diff #405703)

Yeah, I'll also do that.

[ABataev]

In D116637#3294361, @tianshilei1992 wrote:

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

[tianshilei1992]

In D116637#3294390, @ABataev wrote:

In D116637#3294361, @tianshilei1992 wrote:

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

In D116637#3294466, @tianshilei1992 wrote:

In D116637#3294390, @ABataev wrote:

In D116637#3294361, @tianshilei1992 wrote:

@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

[tianshilei1992]

In D116637#3294474, @ABataev wrote:

In D116637#3294466, @tianshilei1992 wrote:

In D116637#3294390, @ABataev wrote:

In D116637#3294361, @tianshilei1992 wrote:

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

In D116637#3294490, @tianshilei1992 wrote:

In D116637#3294474, @ABataev wrote:

In D116637#3294466, @tianshilei1992 wrote:

In D116637#3294390, @ABataev wrote:

In D116637#3294361, @tianshilei1992 wrote:

@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

[tianshilei1992]

In D116637#3294498, @ABataev wrote:

In D116637#3294490, @tianshilei1992 wrote:

In D116637#3294474, @ABataev wrote:

In D116637#3294466, @tianshilei1992 wrote:

In D116637#3294390, @ABataev wrote:

In D116637#3294361, @tianshilei1992 wrote:

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

update test

[tianshilei1992]

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]

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]

clang/test/OpenMP/atomic_ast_print.cpp

964 ↗

(On Diff #405725)

it's caused by the error.

[tianshilei1992]

clang/test/OpenMP/atomic_ast_print.cpp

964 ↗

(On Diff #405725)

Aha, I see. Thanks.

[tianshilei1992]

add more tests

[tianshilei1992]

clang/lib/Sema/SemaOpenMP.cpp

11197

Do you mean by not using auto?

[tianshilei1992]

add final test and fix comments

[ABataev]

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]

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.

[ABataev]

clang/test/OpenMP/atomic_ast_print.cpp

10 ↗

(On Diff #405725)

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

[tianshilei1992]

add TODO for PCH test

[ABataev]

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]

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.

[ABataev]

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]

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]

clang/test/OpenMP/atomic_ast_print.cpp

40 ↗

(On Diff #405967)

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

[tianshilei1992]

move error to code gen and add missing tests

[ABataev]

LG

[cmtice]

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?

[tianshilei1992]

In D116637#3297387, @cmtice wrote:

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.

[kda]

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

[tianshilei1992]

In D116637#3297785, @kda wrote:

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 pushed a fix about half an hour ago. It should fix the issue. Thanks.

[jdoerfert]

In D116637#3297790, @tianshilei1992 wrote:

In D116637#3297785, @kda wrote:

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

[tianshilei1992]

In D116637#3297833, @jdoerfert wrote:

In D116637#3297790, @tianshilei1992 wrote:

In D116637#3297785, @kda wrote:

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

[cmtice]

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.

[tianshilei1992]

In D116637#3298635, @cmtice wrote:

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_omp_atomic_capture_not_expression_statement : Error<
   " where x and v are both lvalue expressions with scalar type">;
 def err_omp_atomic_capture_not_compound_statement : Error<
   "the statement for 'atomic capture' must be a compound statement of form '{v = x; x binop= expr;}', '{x binop= expr; v = x;}',"
   " '{v = x; x = x binop expr;}', '{v = x; x = expr binop x;}', '{x = x binop expr; v = x;}', '{x = expr binop x; v = x;}' or '{v = x; x = expr;}',"
   " '{v = x; x++;}', '{v = x; ++x;}', '{++x; v = x;}', '{x++; v = x;}', '{v = x; x--;}', '{v = x; --x;}', '{--x; v = x;}', '{x--; v = x;}'"
   " where x is an lvalue expression with scalar type">;
 def note_omp_atomic_capture: Note<
   "%select{expected assignment expression|expected compound statement|expected exactly two expression statements|expected in right hand side of the first expression}0">;
+def err_omp_atomic_compare : Error<
+  "the statement for 'atomic compare' must be a compound statement of form '{x = expr ordop x ? expr : x;}', '{x = x ordop expr? expr : x;}',"
+  " '{x = x == e ? d : x;}', '{x = e == x ? d : x;}', or 'if(expr ordop x) {x = expr;}', 'if(x ordop expr) {x = expr;}', 'if(x == e) {x = d;}',"
+  " 'if(e == x) {x = d;}' where 'x' is an lvalue expression with scalar type, 'expr', 'e', and 'd' are expressions with scalar type,"
+  " and 'ordop' is one of '<' or '>'.">;
+def note_omp_atomic_compare: Note<
+  "%select{expected compound statement|expected exactly one expression statement|expected assignment statement|expected conditional operator|expect result value to be at false expression|"
+  "expect binary operator in conditional expression|expect '<', '>' or '==' as order operator|expect comparison in a form of 'x == e', 'e == x', 'x ordop expr', or 'expr ordop x'|"
+  "expect lvalue for result value|expect scalar integer value}0">;
 def err_omp_atomic_several_clauses : Error<
   "directive '#pragma omp atomic' cannot contain more than one 'read', 'write', 'update', 'capture', or 'compare' clause">;
 def err_omp_several_mem_order_clauses : Error<
   "directive '#pragma omp %0' cannot contain more than one %select{'seq_cst', 'relaxed', |}1'acq_rel', 'acquire' or 'release' clause">;
 def err_omp_atomic_incompatible_mem_order_clause : Error<
   "directive '#pragma omp atomic%select{ %0|}1' cannot be used with '%2' clause">;
 def note_omp_previous_mem_order_clause : Note<
   "'%0' clause used here">;

clang/lib/Sema/SemaOpenMP.cpp

if (ErrorFound == NoError && E && X) {
     Update = SemaRef.PerformImplicitConversion(Update.get(), X->getType(),
                                                Sema::AA_Casting);
     if (Update.isInvalid())
       return true;
     UpdateExpr = Update.get();
   }
   return ErrorFound != NoError;
 }
+
+/// Get the node id of the fixed point of an expression \a S.
+llvm::FoldingSetNodeID getNodeId(ASTContext &Context, const Expr *S) {
+  llvm::FoldingSetNodeID Id;
+  S->IgnoreParenImpCasts()->Profile(Id, Context, true);
+  return Id;
+}
+
+/// Check if two expressions are same.
+bool checkIfTwoExprsAreSame(ASTContext &Context, const Expr *LHS,
+                            const Expr *RHS) {
+  return getNodeId(Context, LHS) == getNodeId(Context, RHS);
+}
+
+class OpenMPAtomicCompareChecker {
+public:
+  /// All kinds of errors that can occur in `atomic compare`
+  enum ErrorTy {
+    /// Empty compound statement.
+    NoStmt = 0,
+    /// More than one statement in a compound statement.
+    MoreThanOneStmt,
+    /// Not an assignment binary operator.
+    NotAnAssignment,
+    /// Not a conditional operator.
+    NotCondOp,
+    /// Wrong false expr. According to the spec, 'x' should be at the false
+    /// expression of a conditional expression.
+    WrongFalseExpr,
+    /// The condition of a conditional expression is not a binary operator.
+    NotABinaryOp,
+    /// Invalid binary operator (not <, >, or ==).
+    InvalidBinaryOp,
+    /// Invalid comparison (not x == e, e == x, x ordop expr, or expr ordop x).
+    InvalidComparison,
+    /// X is not a lvalue.
+    XNotLValue,
+    /// Not a scalar integer.
+    NotScalarInteger,
+    /// No error.
+    NoError,
+  };
+
+  struct ErrorInfoTy {
+    ErrorTy Error;
+    SourceLocation ErrorLoc;
+    SourceRange ErrorRange;
+    SourceLocation NoteLoc;
+    SourceRange NoteRange;
+  };
+
+  OpenMPAtomicCompareChecker(Sema &S)
+      : SemaRef(S), ContextRef(S.getASTContext()) {}
+
+  /// Check if statement \a S is valid for <tt>atomic compare</tt>.
+  bool checkStmt(Stmt *S, ErrorInfoTy &ErrorInfo);
+
+  Expr *getX() const { return X; }
+  Expr *getE() const { return E; }
+  Expr *getD() const { return D; }
+  Expr *getCond() const { return C; }
+  bool isSignedOp() const { return IsSignedOp; }
+  bool isXBinopExpr() const { return IsXBinopExpr; }
+
+private:
+  /// Reference to Sema.
+  Sema &SemaRef;
+  /// Reference to ASTContext
+  ASTContext &ContextRef;
+  /// 'x' lvalue part of the source atomic expression.
+  Expr *X = nullptr;
+  /// 'expr' or 'e' rvalue part of the source atomic expression.
+  Expr *E = nullptr;
+  /// 'd' rvalue part of the source atomic expression.
+  Expr *D = nullptr;
+  /// 'cond' part of the source atomic expression. It is in one of the following
+  /// forms:
+  /// expr ordop x
+  /// x ordop expr
+  /// x == e
+  /// e == x
+  Expr *C = nullptr;
+  /// True if the comparison operation is signed.
+  bool IsSignedOp = true;
+  /// True if the cond expr is in the form of 'x ordop expr'.
+  bool IsXBinopExpr = true;
+
+  /// Check if it is a valid conditional update statement (cond-update-stmt).
+  bool checkCondUpdateStmt(IfStmt *S, ErrorInfoTy &ErrorInfo);
+
+  /// Check if it is a valid conditional expression statement (cond-expr-stmt).
+  bool checkCondExprStmt(Stmt *S, ErrorInfoTy &ErrorInfo);
+
+  /// Check if all captured values have right type.
+  bool checkType(ErrorInfoTy &ErrorInfo) const;
+};
+
+bool OpenMPAtomicCompareChecker::checkCondUpdateStmt(IfStmt *S,
+                                                     ErrorInfoTy &ErrorInfo) {
+  auto *Then = S->getThen();
+  if (auto *CS = dyn_cast<CompoundStmt>(Then)) {
+    if (CS->size() == 0) {
+      ErrorInfo.Error = ErrorTy::NoStmt;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = CS->getBeginLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = CS->getSourceRange();
+      return false;
+    }
+    if (CS->size() > 1) {
+      ErrorInfo.Error = ErrorTy::MoreThanOneStmt;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = CS->getBeginLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = S->getSourceRange();
+      return false;
+    }
+    Then = CS->body_front();
+  }
+
+  auto *BO = dyn_cast<BinaryOperator>(Then);
+  if (!BO) {
+    ErrorInfo.Error = ErrorTy::NotAnAssignment;
+    ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Then->getBeginLoc();
+    ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Then->getSourceRange();
+    return false;
+  }
+  if (BO->getOpcode() != BO_Assign) {
+    ErrorInfo.Error = ErrorTy::NotAnAssignment;
+    ErrorInfo.ErrorLoc = BO->getExprLoc();
+    ErrorInfo.NoteLoc = BO->getOperatorLoc();
+    ErrorInfo.ErrorRange = ErrorInfo.NoteRange = BO->getSourceRange();
+    return false;
+  }
+
+  X = BO->getLHS();
+  IsSignedOp = X->getType()->hasSignedIntegerRepresentation();
+
+  auto *Cond = dyn_cast<BinaryOperator>(S->getCond());
+  if (!Cond) {
+    ErrorInfo.Error = ErrorTy::NotABinaryOp;
+    ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = S->getCond()->getExprLoc();
+    ErrorInfo.ErrorRange = ErrorInfo.NoteRange = S->getCond()->getSourceRange();
+    return false;
+  }
+  if (Cond->getOpcode() != BO_EQ && Cond->getOpcode() != BO_LT &&
+      Cond->getOpcode() != BO_GT) {
+    ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
+    ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+    ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+    return false;
+  }
+
+  if (Cond->getOpcode() == BO_EQ) {
+    C = Cond;
+    D = BO->getRHS();
+    if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
+      E = Cond->getRHS();
+    } else if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
+      E = Cond->getLHS();
+    } else {
+      ErrorInfo.Error = ErrorTy::InvalidComparison;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+      return false;
+    }
+  } else {
+    E = BO->getRHS();
+    if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS()) &&
+        checkIfTwoExprsAreSame(ContextRef, E, Cond->getRHS())) {
+      C = Cond;
+    } else if (checkIfTwoExprsAreSame(ContextRef, E, Cond->getLHS()) &&
+               checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
+      C = Cond;
+      IsXBinopExpr = false;
+    } else {
+      ErrorInfo.Error = ErrorTy::InvalidComparison;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+      return false;
+    }
+  }
+
+  return true;
+}
+
+bool OpenMPAtomicCompareChecker::checkCondExprStmt(Stmt *S,
+                                                   ErrorInfoTy &ErrorInfo) {
+  auto *BO = dyn_cast<BinaryOperator>(S);
+  if (!BO) {
+    ErrorInfo.Error = ErrorTy::NotAnAssignment;
+    ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = S->getBeginLoc();
+    ErrorInfo.ErrorRange = ErrorInfo.NoteRange = S->getSourceRange();
+    return false;
+  }
+  if (BO->getOpcode() != BO_Assign) {
+    ErrorInfo.Error = ErrorTy::NotAnAssignment;
+    ErrorInfo.ErrorLoc = BO->getExprLoc();
+    ErrorInfo.NoteLoc = BO->getOperatorLoc();
+    ErrorInfo.ErrorRange = ErrorInfo.NoteRange = BO->getSourceRange();
+    return false;
+  }
+
+  X = BO->getLHS();
+  IsSignedOp = X->getType()->hasSignedIntegerRepresentation();
+
+  auto *CO = dyn_cast<ConditionalOperator>(BO->getRHS()->IgnoreParenImpCasts());
+  if (!CO) {
+    ErrorInfo.Error = ErrorTy::NotCondOp;
+    ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = BO->getRHS()->getExprLoc();
+    ErrorInfo.ErrorRange = ErrorInfo.NoteRange = BO->getRHS()->getSourceRange();
+    return false;
+  }
+
+  if (!checkIfTwoExprsAreSame(ContextRef, X, CO->getFalseExpr())) {
+    ErrorInfo.Error = ErrorTy::WrongFalseExpr;
+    ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = CO->getFalseExpr()->getExprLoc();
+    ErrorInfo.ErrorRange = ErrorInfo.NoteRange =
+        CO->getFalseExpr()->getSourceRange();
+    return false;
+  }
+
+  auto *Cond = dyn_cast<BinaryOperator>(CO->getCond());
+  if (!Cond) {
+    ErrorInfo.Error = ErrorTy::NotABinaryOp;
+    ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = CO->getCond()->getExprLoc();
+    ErrorInfo.ErrorRange = ErrorInfo.NoteRange =
+        CO->getCond()->getSourceRange();
+    return false;
+  }
+
+  if (Cond->getOpcode() != BO_EQ && Cond->getOpcode() != BO_LT &&
+      Cond->getOpcode() != BO_GT) {
+    ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
+    ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+    ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+    return false;
+  }
+
+  if (Cond->getOpcode() == BO_EQ) {
+    C = Cond;
+    D = CO->getTrueExpr();
+    if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
+      E = Cond->getRHS();
+    } else if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
+      E = Cond->getLHS();
+    } else {
+      ErrorInfo.Error = ErrorTy::InvalidComparison;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+      return false;
+    }
+  } else {
+    E = CO->getTrueExpr();
+    if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS()) &&
+        checkIfTwoExprsAreSame(ContextRef, E, Cond->getRHS())) {
+      C = Cond;
+    } else if (checkIfTwoExprsAreSame(ContextRef, E, Cond->getLHS()) &&
+               checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
+      C = Cond;
+      IsXBinopExpr = false;
+    } else {
+      ErrorInfo.Error = ErrorTy::InvalidComparison;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+      return false;
+    }
+  }
+
+  return true;
+}
+
+bool OpenMPAtomicCompareChecker::checkType(ErrorInfoTy &ErrorInfo) const {
+  // 'x' and 'e' cannot be nullptr
+  assert(X && E && "X and E cannot be nullptr");
+
+  {
+    if (!X->isLValue()) {
+      ErrorInfo.Error = ErrorTy::XNotLValue;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = X->getExprLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = X->getSourceRange();
+      return false;
+    }
+
+    auto Type = X->getType();
+    if (!Type->isScalarType() || !Type->isIntegerType()) {
+      ErrorInfo.Error = ErrorTy::NotScalarInteger;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = X->getExprLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = X->getSourceRange();
+      return false;
+    }
+  }
+
+  {
+    auto Type = E->getType();
+    if (!Type->isScalarType() || !Type->isIntegerType()) {
+      ErrorInfo.Error = ErrorTy::NotScalarInteger;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = E->getExprLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = E->getSourceRange();
+      return false;
+    }
+  }
+
+  if (D) {
+    auto Type = D->getType();
+    if (!Type->isScalarType() || !Type->isIntegerType()) {
+      ErrorInfo.Error = ErrorTy::NotScalarInteger;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = D->getExprLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = D->getSourceRange();
+      return false;
+    }
+  }
+
+  return true;
+}
+
+bool OpenMPAtomicCompareChecker::checkStmt(
+    Stmt *S, OpenMPAtomicCompareChecker::ErrorInfoTy &ErrorInfo) {
+  auto *CS = dyn_cast<CompoundStmt>(S);
+  if (CS) {
+    if (CS->size() == 0) {
+      ErrorInfo.Error = ErrorTy::NoStmt;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = CS->getBeginLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = CS->getSourceRange();
+      return false;
+    }
+
+    if (CS->size() != 1) {
+      ErrorInfo.Error = ErrorTy::MoreThanOneStmt;
+      ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = CS->getBeginLoc();
+      ErrorInfo.ErrorRange = ErrorInfo.NoteRange = CS->getSourceRange();
+      return false;
+    }
+    S = CS->body_front();
+  }
+
+  auto Res = false;
+
+  if (auto *IS = dyn_cast<IfStmt>(S)) {
+    // Check if the statement is in one of the following forms
+    // (cond-update-stmt):
+    // if (expr ordop x) { x = expr; }
+    // if (x ordop expr) { x = expr; }
+    // if (x == e) { x = d; }
+    Res = checkCondUpdateStmt(IS, ErrorInfo);
+  } else {
+    // Check if the statement is in one of the following forms (cond-expr-stmt):
+    // x = expr ordop x ? expr : x;
+    // x = x ordop expr ? expr : x;
+    // x = x == e ? d : x;
+    Res = checkCondExprStmt(S, ErrorInfo);
+  }
+
+  if (!Res)
+    return false;
+
+  return checkType(ErrorInfo);
+}
 } // namespace
 
 StmtResult Sema::ActOnOpenMPAtomicDirective(ArrayRef<OMPClause *> Clauses,
                                             Stmt *AStmt,
                                             SourceLocation StartLoc,
                                             SourceLocation EndLoc) {
   // Register location of the first atomic directive.
   DSAStack->addAtomicDirectiveLoc(StartLoc);
if (ErrorFound != NoError) {
       Diag(ErrorLoc, diag::err_omp_atomic_capture_not_compound_statement)
           << ErrorRange;
       Diag(NoteLoc, diag::note_omp_atomic_capture) << ErrorFound << NoteRange;
       return StmtError();
     }
     if (CurContext->isDependentContext())
       UE = V = E = X = nullptr;
   } else if (AtomicKind == OMPC_compare) {
+    OpenMPAtomicCompareChecker::ErrorInfoTy ErrorInfo;
+    OpenMPAtomicCompareChecker Checker(*this);
+    if (!Checker.checkStmt(Body, ErrorInfo)) {
+      Diag(ErrorInfo.ErrorLoc, diag::err_omp_atomic_compare)
+          << ErrorInfo.ErrorRange;
+      Diag(ErrorInfo.NoteLoc, diag::note_omp_atomic_compare)
+          << ErrorInfo.Error << ErrorInfo.NoteRange;
+      return StmtError();
+    }
     // TODO: For now we emit an error here and in emitOMPAtomicExpr we ignore
     // code gen.
     unsigned DiagID = Diags.getCustomDiagID(
         DiagnosticsEngine::Error, "atomic compare is not supported for now");
     Diag(AtomicKindLoc, DiagID);
   }
 
   setFunctionHasBranchProtectedScope();