[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Shilei Tian via Phabricator via llvm-commits]

tianshilei1992 added a comment.

In D123235#3513430 <https://reviews.llvm.org/D123235#3513430>, @koops wrote:

> I have tried on x64 RH and x64 SuSe. I could not reproduce the failures seen on x64 debian.  https://reviews.llvm.org/D118550 also has similar failures on x64 debian. There is a comment " I think the test failures are spurious (but not 100% sure)"  So, are these failures pre-existing before the changes in the current support for "atomic compare fail: Parser & Support" were done?

Those tests should be irrelevant.



================
Comment at: clang/include/clang/AST/ASTNodeTraverser.h:228
 
+  void Visit(const OMPFailClause *C) {
+    getNodeDelegate().AddChild([=] {
----------------
koops wrote:
> tianshilei1992 wrote:
> > Why would we want a dedicated function since it is only called once?
> The code for this method cannot be put into any other method because it handles only OMPFailClause. All other Visit methods handle either the generalized OMPClause or other types of Clauses.
I mean, it's only used by the function above, no?


================
Comment at: clang/include/clang/AST/ASTNodeTraverser.h:231-232
+      getNodeDelegate().Visit(C);
+      const OMPClause *mOC = C->const_getMemoryOrderClause();
+      Visit(mOC);
+    });
----------------



================
Comment at: clang/include/clang/Parse/Parser.h:434
 
+  OMPClause *ParseOpenMPFailClause(OMPClause *clause);
+
----------------



================
Comment at: clang/lib/AST/OpenMPClause.cpp:417-432
+OMPFailClause *OMPFailClause::Create(const ASTContext &C,
+                                     SourceLocation StartLoc,
+                                     SourceLocation EndLoc) {
+  void *Mem =
+      C.Allocate(totalSizeToAlloc<SourceLocation, OpenMPClauseKind>(2, 1), alignof(OMPFailClause));
+  auto *Clause =
+      new (Mem) OMPFailClause(StartLoc, EndLoc);
----------------
clang-format plz


================
Comment at: clang/lib/Basic/OpenMPKinds.cpp:370
+  case OMPC_fail: {
+    OpenMPClauseKind ck = static_cast<OpenMPClauseKind>(Type);
+    switch (ck) {
----------------
maybe something like `CK`? `ck` doesn't conform with LLVM standard.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:3622
+
+  OMPFailClause *failClause = static_cast<OMPFailClause *>(clause);
+  SourceLocation LParenLoc;
----------------



================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:3691
     return nullptr;
-  return Actions.ActOnOpenMPClause(Kind, Loc, Tok.getLocation());
+  OMPClause *clause = Actions.ActOnOpenMPClause(Kind, Loc, Tok.getLocation());
+  if (Kind == llvm::omp::Clause::OMPC_fail) {
----------------
ditto


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11999
+                                             SourceLocation *ErrorLoc) {
+  int no_of_fails = 0;
+  ErrorNo = 0;
----------------
ditto


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

https://reviews.llvm.org/D123235