[PATCH] D91944: OpenMP 5.0 metadirective

[Alok Mishra via Phabricator via cfe-commits]

alokmishra.besu added a comment.

I have replied to the comments and will update the code accordingly. Some of the codes are intentionally left to be update in 5.1 implementation. Will add TODO comments there.
I will revisit the implementation of getBestWhenMatchForContext and also add more test cases over the weekend and submit new code by next week.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10493
+  "misplaced default clause! Only one default clause is allowed in "
+  "metadirective in the end">;
 } // end of OpenMP category
----------------
jdoerfert wrote:
> no `!`. The default clause doesn't need to be in the end either.
OK. Will update accordingly.


================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1952
-      STMT_MS_DEPENDENT_EXISTS,   // MSDependentExistsStmt
-      EXPR_LAMBDA,                // LambdaExpr
       STMT_COROUTINE_BODY,
----------------
jdoerfert wrote:
> Unrelated.
Only STMT_OMP_META_DIRECTIVE was added. Rest was formatted by git clang-format


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:10211
+    return;
+  }
+
----------------
jdoerfert wrote:
> Can you explain this, this seems odd to me.
Good catch. This was an experimental code for 5.1. Got committed by mistake. Will update.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2186
+        // parse and get condition expression to pass to the When clause
+        parseOMPContextSelectors(Loc, TI);
+
----------------
jdoerfert wrote:
> Usually you would check the return value in case we later actually propagate errors while parsing the context selector.
OK. Will update accordingly.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2193
+          Diag(Tok, diag::warn_pragma_expected_colon) << "when clause";
+          return Directive;
+        }
----------------
jdoerfert wrote:
> If we give up it should be an error, I think. If we issue a warning we just pretend the colon was there afterwards.
OK. Will update accordingly.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2204
+        ConsumeAnyToken();
+      }
+      // Parse ')'
----------------
jdoerfert wrote:
> We have balanced trackers for this that also deal with the fact that there might be a `)` missing. This code will probably crash.
Come to think of it, in case of a missing ')' this code might end up in an infinite loop. Will update accordingly.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2216
+    TPA.Revert();
+    TargetOMPContext OMPCtx(ASTContext, nullptr, nullptr);
+    int BestIdx = getBestWhenMatchForContext(VMIs, OMPCtx);
----------------
jdoerfert wrote:
> Add a TODO that `nullptr` should be replaced as per the use in `Sema::ActOnOpenMPCall`.
Will do


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2254
+        // Parse ':'
+        ConsumeAnyToken();
+      }
----------------
jdoerfert wrote:
> If you warn and continue above you need to check for `:` here again.
Will update accordingly.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2264
+          Actions.StartOpenMPDSABlock(DirKind, DirName, Actions.getCurScope(),
+                                      Loc);
+          int paren = 0;
----------------
jdoerfert wrote:
> Should we not go back to the original code handling "directives" instead? This looks like it is copied here.
Unfortunately we cannot go to the original code handling since the original code handling assumes that the directive always ends with annot_pragma_openmp_end, while here it will always end with ')'.
In specification 5.0, since we are choosing only 1 directive, the body of the while block remains the same as the original code. Only the condition of the while block changes. In specification 5.1, we will need to generate code for dynamic handling and even the body will differ as we might need to generate AST node for multiple directives. It is best if we handle this code here for easier handling of 5.1 code, than in the original code space.
I will add a TODO comment here.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2317
+          ConsumeAnnotationToken();
+        }
+      } else {
----------------
jdoerfert wrote:
> Same as below, change the order. Also, the "skipping" part is always the same, put it in a helper function or lambda.
OK


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2324
+        ConsumeAnnotationToken();
+      }
+      break;
----------------
jdoerfert wrote:
> Move the smaller case first and use an early exit. That will reduce the indention of the larger case by 1.
OK


================
Comment at: clang/test/OpenMP/metadirective_empty.cpp:1
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
----------------
jdoerfert wrote:
> no -fopenmp-targets please.
OK


================
Comment at: clang/test/OpenMP/metadirective_implementation.cpp:1
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
----------------
jdoerfert wrote:
> Can we run this for all configurations of the metadirective so we can actually see it will pick the right one, not the first?
Good catch. I will go through the implementation of getBestWhenMatchForContext to check that.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPContext.h:197
+// int getBestWhenMatchForContext(const SmallVectorImpl<VariantMatchInfo> &VMIs,
+//                               const OMPContext &Ctx);
 /// Return the index (into \p VMIs) of the variant with the highest score
----------------
jdoerfert wrote:
> Leftover.
Will remove.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:337
+    const SmallVectorImpl<VariantMatchInfo> &VMIs, const OMPContext &Ctx,
+    SmallVectorImpl<unsigned> *OrderedMatch) {
+
----------------
jdoerfert wrote:
> This looks like a clone of `getBestVariantMatchForContext` with an extra unused argument.
I intended to keep it similar for 5.0 to be updated in 5.1 code. But anyways it seems to be giving wrong result. Will go through this again.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:400
+  return BestVMIIdx;
+}*/
+
----------------
jdoerfert wrote:
> Leftover.
Will remove.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91944