[PATCH] D116261: [Clang][OpenMP] Add support for compare capture in parser

[Shilei Tian via Phabricator via cfe-commits]

tianshilei1992 marked an inline comment as done.
tianshilei1992 added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11801-11807
+  } else if (AtomicKind == OMPC_compare_capture) {
+    // 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);
----------------
ABataev wrote:
> tianshilei1992 wrote:
> > ABataev wrote:
> > > tianshilei1992 wrote:
> > > > ABataev wrote:
> > > > > 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.
> > > > 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.
> > > Then why do you need OMPC_compare_capture? Just emit compare and capture and check for both of them at the same time.
> > `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.
> 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.
Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116261/new/

https://reviews.llvm.org/D116261