[PATCH] D122255: Meta directive runtime support

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:8859
+/// \code
+/// #pragma omp metadirective when(user={consition(N<100)}:parallel for)
+/// \endcode
----------------
condition?


================
Comment at: clang/include/clang/AST/OpenMPClause.h:8893-8894
+
+  /// Sets the location of '('.
+  void setLParenLoc(SourceLocation Loc) { LParenLoc = Loc; }
+
----------------
make it private


================
Comment at: clang/include/clang/AST/OpenMPClause.h:8900
+  /// Returns the directive variant kind
+  OpenMPDirectiveKind getDKind() { return DKind; }
+
----------------
const


================
Comment at: clang/include/clang/AST/OpenMPClause.h:8905
+  /// Returns the OMPTraitInfo
+  OMPTraitInfo &getTI() { return *TI; }
+
----------------
const


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:5479
                                        EmptyShell);
+                                       
   Stmt *getIfStmt() const { return IfStmt; }
----------------
Remove this change


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10852
+def err_omp_misplaced_default_clause : Error<
+  "misplaced default clause! Only one default clause is allowed in"
+  "metadirective in the end">;
----------------
We do not use explamations in messages, better to have something `only single default ...`


================
Comment at: clang/include/clang/Sema/Sema.h:10688
       Stmt *AStmt, SourceLocation StartLoc, SourceLocation EndLoc);
+  StmtResult ActOnOpenMPExecutableMetaDirective( 
+      OpenMPDirectiveKind Kind, const DeclarationNameInfo &DirName, 
----------------
Comment for this new function


================
Comment at: clang/include/clang/Sema/Sema.h:11138
+  OMPClause *ActOnOpenMPWhenClause(OMPTraitInfo &TI, OpenMPDirectiveKind DKind,
+  				    StmtResult Directive,	 
+  				    SourceLocation StartLoc,
----------------
formatting


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1614-1617
+  if (Node->getTI().Sets.size() == 0) {
+    OS << "default(";
+    return;
+  }
----------------
Is this correct? Just `default(` is expected to be printed?


================
Comment at: clang/lib/AST/StmtPrinter.cpp:658
                                               bool ForceNoStmt) {
+    
   OMPClausePrinter Printer(OS, Policy);
----------------
Do not insert empty line here


================
Comment at: clang/lib/AST/StmtPrinter.cpp:661
   ArrayRef<OMPClause *> Clauses = S->clauses();
-  for (auto *Clause : Clauses)
+  for (auto *Clause : Clauses){    
     if (Clause && !Clause->isImplicit()) {
----------------
formatting


================
Comment at: clang/lib/AST/StmtPrinter.cpp:665
       Printer.Visit(Clause);
+      if (dyn_cast<OMPMetaDirective>(S)){
+     
----------------
use `isa`


================
Comment at: clang/lib/AST/StmtPrinter.cpp:667
+     
+      	OMPWhenClause *c = dyn_cast<OMPWhenClause>(Clause);
+      	if (c!=NULL){
----------------
Camel naming style expceted


================
Comment at: clang/lib/AST/StmtPrinter.cpp:668
+      	OMPWhenClause *c = dyn_cast<OMPWhenClause>(Clause);
+      	if (c!=NULL){
+      		if (c->getDKind() != llvm::omp::OMPD_unknown){
----------------
formatting, use nullptr instead of NULL (or just `C`)


================
Comment at: clang/lib/AST/StmtPrinter.cpp:669
+      	if (c!=NULL){
+      		if (c->getDKind() != llvm::omp::OMPD_unknown){
+      			Printer.VisitOMPWhenClause(c);
----------------
Formatting


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2433
 Parser::ParseOpenMPDeclarativeOrExecutableDirective(ParsedStmtContext StmtCtx) {
+// need to check about the following
   static bool ReadDirectiveWithinMetadirective = false;
----------------
What is this?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2474
                                tok::annot_pragma_openmp_end);
+                               
     while (Tok.isNot(tok::annot_pragma_openmp_end)) {
----------------
Restore original code here


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2498-2506
+        // Parse ':' // You have parsed the OpenMP Context in the meta directive 
         if (Tok.is(tok::colon))
           ConsumeAnyToken();
         else {
           Diag(Tok, diag::err_omp_expected_colon) << "when clause";
           TPA.Commit();
           return Directive;
----------------
Remove this extra stuff 


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2546
+    // Array A will be used for sorting                        
+    SmallVector<std::pair<unsigned, llvm::APInt>> A;
+    
----------------
What `A` stands after, what does it mean?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122255