[Clang][OpenMP] Add support for compare capture in parser

[tianshilei1992]

Summary

This patch adds the support for atomic compare capture in parser and part of
sema. We don't create an AST node for this because the spec doesn't say compare
and capture clauses should be used tightly, so we cannot look one more token
ahead in the parser.

[tianshilei1992]

clang/lib/Sema/SemaOpenMP.cpp

11368

This is the place where the dummy kind OMPC_compare_capture is used. We will have another use in CodeGen.

[tianshilei1992]

update test case

[tianshilei1992]

rebase

[ABataev]

clang/include/clang/AST/OpenMPClause.h

2277 ↗

(On Diff #406867)

final

clang/lib/AST/OpenMPClause.cpp

1804 ↗

(On Diff #406867)

Output?

clang/lib/Sema/SemaOpenMP.cpp

11327

Use LLVM_FALLTHROUGH

11362–11363

Why?

11812–11817

Emit in codegen

[tianshilei1992]

clang/lib/AST/OpenMPClause.cpp

1804 ↗

(On Diff #406867)

I did it on purpose because OMPCompareCaptureClause is a dummy node. When it is visited (I really doubt if it can be visited because we don't create it explicitly), it should not output any thing. compare and capture are printed when visiting the two clauses.

clang/lib/Sema/SemaOpenMP.cpp

11362–11363

For the same reason for the output in AST.

[ABataev]

clang/lib/AST/OpenMPClause.cpp

1804 ↗

(On Diff #406867)

Why? I see ActOnOpenMPCompareCaptureClause, which creates such nodes.

[tianshilei1992]

rebase and mark related functions as unreachable

[tianshilei1992]

clang/lib/AST/OpenMPClause.cpp

1804 ↗

(On Diff #406867)

I updated those functions. They should be unreachable.

clang/lib/Sema/SemaOpenMP.cpp

11812–11817

Since we are lack of Sema, putting it in Sema for now sounds more reasonable.

[tianshilei1992]

use LLVM_FALLTHROUGH

[tianshilei1992]

cleanup

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

11806–11812

Better to build a node and emit error in codegen. Without it you're unable to create ast-print/dump tests, test for serialization/deserialization etc.

[tianshilei1992]

clang/lib/Sema/SemaOpenMP.cpp

11806–11812

IIUC, OMPC_compare_capture will only be used in Sema and CodeGen to tell we actually want compare_capture instead of compare or capture. The corresponding class(es) have no actual use. If I could directly have OMPC_compare_capture w/o adding a class, I would like to do it. On the other hand, we already have a node for compare, and a node for capture, we don't want a node for compare and capture, especially the spec doesn't say they should be tightly close. That being said, it should not affect other functionality.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

11806–11812

Then why do you need OMPC_compare_capture? Just emit compare and capture and check for both of them at the same time.

[tianshilei1992]

clang/lib/Sema/SemaOpenMP.cpp

11806–11812

OMPC_compare_capture is the natural way, especially when we call emitOMPAtomicExpr. Of course we can set Kind to OMPC_compare and add a boolean argument bool IsCompareCapture = false. It's just not as clear as this way, but TBH not a bad way.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

11806–11812

I think it is better to have a flag (or an internal enum) rather than adding an extra external enum and a class just for internal processing.

[tianshilei1992]

remove the dummy node

[tianshilei1992]

clang/lib/Sema/SemaOpenMP.cpp

11806–11812

Done.

[tianshilei1992]

fix errors

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

11786–11805

Better not to emit error here, if possible. What's prevent this?

[tianshilei1992]

clang/lib/Sema/SemaOpenMP.cpp

11786–11805

I put it here because I think we don't have Sema support yet, so it is not good to move it to later phase like codegen because that will implicitly mean Sema has already passed but apparently it doesn't. Of course like atomic compare, I'll move it to codegen in the Sema patch.

[ABataev]

Add ast print with serialization/deserialization tests.

clang/lib/Sema/SemaOpenMP.cpp

11320

SmallVector with 2 elements should be enough, no need to use SmallSet here.

[tianshilei1992]

clang/lib/Sema/SemaOpenMP.cpp

11320

SmallVector doesn't provide contains, which we use to check if OMPC_compare and OMPC_capture exist at the same time. Using SmallVector requires us to have a complicated comparison, for example:

(EncounteredAtomicKinds[0] == OMPC_compare && EncounteredAtomicKinds[1] == OMPC_capture) || (EncounteredAtomicKinds[1] == OMPC_compare && EncounteredAtomicKinds[0] == OMPC_capture)

which is not as neat as SmallSet.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

11320

Use llvm::find

[tianshilei1992]

clang/lib/Sema/SemaOpenMP.cpp

11320

That is linear time. Why it is better than SmallSet?

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

11320

Because there are only 2 elements, no?

[tianshilei1992]

clang/lib/Sema/SemaOpenMP.cpp

11320

It could have more. If user writes it like atomic compare compare capture capture capture, then it will have 5 elements. SmallSet<T>::insert returns a pair and we know if it is already in the set, which I have not added it yet but I'll. SmallVector then is not that easy to use. Of course you could say do llvm::find beforehand but it's not neat.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

11320

Is this correct at all?

11320

You still can scan over the small vector and check if it has specified clause already.

[tianshilei1992]

clang/lib/Sema/SemaOpenMP.cpp

11320

It's not correct. We can easily check via the following code:

if (!EncounteredAtomicKinds.insert(C->getClauseKind()).second) {
  // emit an error
}

Using SmallVector we need to:

if (llvm::find(EncounteredAtomicKinds.begin(), EncounteredAtomicKinds.end(), C->getClauseKind() != EncounteredAtomicKinds.end()) {
  // emit an error
} else {
  EncounteredAtomicKinds.push_back(C->getClauseKind());
}

It's obvious which one looks better and have better readability. I can't see any point of using SmallVector here, unless there is huge performance difference, which I really doubt that matters.

[tianshilei1992]

add tests

[ABataev]

Erroneous tests?

clang/lib/Sema/SemaOpenMP.cpp

11320

if (llvm::find(EncounteredAtomicKinds, C->getClauseKind() != EncounteredAtomicKinds.end()) {
  // emit an error
} else {
  EncounteredAtomicKinds.push_back(C->getClauseKind());
}

but ok, let's keep SmallSet

[tianshilei1992]

We don't have Sema yet, so there is no erroneous test.

[ABataev]

In D116261#3330732, @tianshilei1992 wrote:

We don't have Sema yet, so there is no erroneous test.

But you have the checks for the clauses, repeated several times in a construct.

[tianshilei1992]

add a small test

[ABataev]

LG

clang/lib/CodeGen/CGStmtOpenMP.cpp

 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclOpenMP.h"
 #include "clang/AST/OpenMPClause.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtOpenMP.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/OpenMPKinds.h"
 #include "clang/Basic/PrettyStackTrace.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Metadata.h"
if (CGF.CGM.getLangOpts().OpenMP < 51) {
     }
   }
 }
 
 static void emitOMPAtomicExpr(CodeGenFunction &CGF, OpenMPClauseKind Kind,
                               llvm::AtomicOrdering AO, bool IsPostfixUpdate,
                               const Expr *X, const Expr *V, const Expr *E,
                               const Expr *UE, bool IsXLHSInRHSPart,
-                              SourceLocation Loc) {
+                              bool IsCompareCapture, SourceLocation Loc) {
   switch (Kind) {
   case OMPC_read:
     emitOMPAtomicReadExpr(CGF, AO, X, V, Loc);
     break;
   case OMPC_write:
     emitOMPAtomicWriteExpr(CGF, AO, X, E, Loc);
     break;
   case OMPC_unknown:
   case OMPC_update:
     emitOMPAtomicUpdateExpr(CGF, AO, X, E, UE, IsXLHSInRHSPart, Loc);
     break;
   case OMPC_capture:
     emitOMPAtomicCaptureExpr(CGF, AO, IsPostfixUpdate, V, X, E, UE,
                              IsXLHSInRHSPart, Loc);
     break;
   case OMPC_compare: {
+    if (!IsCompareCapture) {
       // Emit an error here.
       unsigned DiagID = CGF.CGM.getDiags().getCustomDiagID(
-        DiagnosticsEngine::Error, "'atomic compare' is not supported for now");
+          DiagnosticsEngine::Error,
+          "'atomic compare' is not supported for now");
       CGF.CGM.getDiags().Report(DiagID);
+    }
     break;
   }
   case OMPC_if:
   case OMPC_final:
   case OMPC_num_threads:
   case OMPC_private:
   case OMPC_firstprivate:
   case OMPC_lastprivate:
if (S.getSingleClause<OMPSeqCstClause>()) {
     MemOrderingSpecified = true;
   } else if (S.getSingleClause<OMPReleaseClause>()) {
     AO = llvm::AtomicOrdering::Release;
     MemOrderingSpecified = true;
   } else if (S.getSingleClause<OMPRelaxedClause>()) {
     AO = llvm::AtomicOrdering::Monotonic;
     MemOrderingSpecified = true;
   }
+  llvm::SmallSet<OpenMPClauseKind, 2> KindsEncountered;
   OpenMPClauseKind Kind = OMPC_unknown;
   for (const OMPClause *C : S.clauses()) {
     // Find first clause (skip seq_cst|acq_rel|aqcuire|release|relaxed clause,
     // if it is first).
-    if (C->getClauseKind() != OMPC_seq_cst &&
-        C->getClauseKind() != OMPC_acq_rel &&
-        C->getClauseKind() != OMPC_acquire &&
-        C->getClauseKind() != OMPC_release &&
-        C->getClauseKind() != OMPC_relaxed && C->getClauseKind() != OMPC_hint) {
-      Kind = C->getClauseKind();
-      break;
-    }
+    OpenMPClauseKind K = C->getClauseKind();
+    if (K == OMPC_seq_cst || K == OMPC_acq_rel || K == OMPC_acquire ||
+        K == OMPC_release || K == OMPC_relaxed || K == OMPC_hint)
+      continue;
+    Kind = K;
+    KindsEncountered.insert(K);
   }
+  bool IsCompareCapture = KindsEncountered.contains(OMPC_compare) &&
+                          KindsEncountered.contains(OMPC_capture);
   if (!MemOrderingSpecified) {
     llvm::AtomicOrdering DefaultOrder =
         CGM.getOpenMPRuntime().getDefaultMemoryOrdering();
     if (DefaultOrder == llvm::AtomicOrdering::Monotonic ||
         DefaultOrder == llvm::AtomicOrdering::SequentiallyConsistent ||
         (DefaultOrder == llvm::AtomicOrdering::AcquireRelease &&
          Kind == OMPC_capture)) {
       AO = DefaultOrder;
     } else if (DefaultOrder == llvm::AtomicOrdering::AcquireRelease) {
       if (Kind == OMPC_unknown || Kind == OMPC_update || Kind == OMPC_write) {
         AO = llvm::AtomicOrdering::Release;
       } else if (Kind == OMPC_read) {
         assert(Kind == OMPC_read && "Unexpected atomic kind.");
         AO = llvm::AtomicOrdering::Acquire;
       }
     }
   }
 
   LexicalScope Scope(*this, S.getSourceRange());
   EmitStopPoint(S.getAssociatedStmt());
   emitOMPAtomicExpr(*this, Kind, AO, S.isPostfixUpdate(), S.getX(), S.getV(),
                     S.getExpr(), S.getUpdateExpr(), S.isXLHSInRHSPart(),
-                    S.getBeginLoc());
+                    IsCompareCapture, S.getBeginLoc());
 }
 
 static void emitCommonOMPTargetDirective(CodeGenFunction &CGF,
                                          const OMPExecutableDirective &S,
                                          const RegionCodeGenTy &CodeGen) {
   assert(isOpenMPTargetExecutionDirective(S.getDirectiveKind()));
   CodeGenModule &CGM = CGF.CGM;
 

clang/lib/Sema/SemaOpenMP.cpp

 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "llvm/ADT/IndexedMap.h"
 #include "llvm/ADT/PointerEmbeddedInt.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Frontend/OpenMP/OMPAssume.h"
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include <set>
 
 using namespace clang;
 using namespace llvm::omp;
 
StmtResult Sema::ActOnOpenMPAtomicDirective(ArrayRef<OMPClause *> Clauses,
   // Structured block - An executable statement with a single entry at the
   // top and a single exit at the bottom.
   // The point of exit cannot be a branch out of the structured block.
   // longjmp() and throw() must not violate the entry/exit criteria.
   OpenMPClauseKind AtomicKind = OMPC_unknown;
   SourceLocation AtomicKindLoc;
   OpenMPClauseKind MemOrderKind = OMPC_unknown;
   SourceLocation MemOrderLoc;
+  bool MutexClauseEncountered = false;
+  llvm::SmallSet<OpenMPClauseKind, 2> EncounteredAtomicKinds;
   for (const OMPClause *C : Clauses) {
     switch (C->getClauseKind()) {
     case OMPC_read:
     case OMPC_write:
     case OMPC_update:
+      MutexClauseEncountered = true;
+      LLVM_FALLTHROUGH;
     case OMPC_capture:
     case OMPC_compare: {
-      if (AtomicKind != OMPC_unknown) {
+      if (AtomicKind != OMPC_unknown && MutexClauseEncountered) {
         Diag(C->getBeginLoc(), diag::err_omp_atomic_several_clauses)
             << SourceRange(C->getBeginLoc(), C->getEndLoc());
         Diag(AtomicKindLoc, diag::note_omp_previous_mem_order_clause)
             << getOpenMPClauseName(AtomicKind);
       } else {
         AtomicKind = C->getClauseKind();
         AtomicKindLoc = C->getBeginLoc();
+        EncounteredAtomicKinds.insert(C->getClauseKind());
       }
       break;
     }
     case OMPC_seq_cst:
     case OMPC_acq_rel:
     case OMPC_acquire:
     case OMPC_release:
     case OMPC_relaxed: {
       if (MemOrderKind != OMPC_unknown) {
         Diag(C->getBeginLoc(), diag::err_omp_several_mem_order_clauses)
             << getOpenMPDirectiveName(OMPD_atomic) << 0
             << SourceRange(C->getBeginLoc(), C->getEndLoc());
         Diag(MemOrderLoc, diag::note_omp_previous_mem_order_clause)
             << getOpenMPClauseName(MemOrderKind);
       } else {
         MemOrderKind = C->getClauseKind();
         MemOrderLoc = C->getBeginLoc();
       }
       break;
     }
     // The following clauses are allowed, but we don't need to do anything here.
     case OMPC_hint:
       break;
     default:
       llvm_unreachable("unknown clause is encountered");
     }
   }
+  bool IsCompareCapture = EncounteredAtomicKinds.contains(OMPC_compare) &&
+                          EncounteredAtomicKinds.contains(OMPC_capture);
   // OpenMP 5.0, 2.17.7 atomic Construct, Restrictions
   // If atomic-clause is read then memory-order-clause must not be acq_rel or
   // release.
   // If atomic-clause is write then memory-order-clause must not be acq_rel or
   // acquire.
   // If atomic-clause is update or not present then memory-order-clause must not
   // be acq_rel or acquire.
   if ((AtomicKind == OMPC_read &&
        (MemOrderKind == OMPC_acq_rel || MemOrderKind == OMPC_release)) ||
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) {
+    if (IsCompareCapture) {
+      // TODO: For now we emit an error here and in emitOMPAtomicExpr we ignore
+      // code gen.
+      unsigned DiagID = Diags.getCustomDiagID(
+          DiagnosticsEngine::Error,
+          "atomic compare capture is not supported for now");
+      Diag(AtomicKindLoc, DiagID);
+    } else {
       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: We don't set X, D, E, etc. here because in code gen we will emit
       // error directly.
     }
+  }
 
   setFunctionHasBranchProtectedScope();
 
   return OMPAtomicDirective::Create(Context, StartLoc, EndLoc, Clauses, AStmt,
                                     X, V, E, UE, IsXLHSInRHSPart,
                                     IsPostfixUpdate);
 }
 
 StmtResult Sema::ActOnOpenMPTargetDirective(ArrayRef<OMPClause *> Clauses,
                                             Stmt *AStmt,
                                             SourceLocation StartLoc,
                                             SourceLocation EndLoc) {
   if (!AStmt)
     return StmtError();
 
   auto *CS = cast<CapturedStmt>(AStmt);
   // 1.2.2 OpenMP Language Terminology
   // Structured block - An executable statement with a single entry at the
   // top and a single exit at the bottom.

clang/test/OpenMP/atomic_messages.cpp

 // expected-error@+2 {{directive '#pragma omp atomic' cannot contain more than one 'read', 'write', 'update', 'capture', or 'compare' clause}}
 // expected-note@+1 {{'read' clause used here}}
 #pragma omp atomic read compare
   a = b;
 #endif
   // expected-note@+1 {{in instantiation of function template specialization 'mixed<int>' requested here}}
   return mixed<int>();
 }
+
+#ifdef OMP51
+int compare_capture() {
+  int a, b, c, x;
+// omp51-error@+1 {{atomic compare capture is not supported for now}}
+#pragma omp atomic compare capture
+  {
+    x = a;
+    if (a == b)
+      a = c;
+  }
+}
+#endif