[PATCH] D122255: Meta directive runtime support

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
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">;
----------------
ABataev wrote:
> We do not use explamations in messages, better to have something `only single default ...`
"allowed in" what? Also, there is no test for this, or is there?


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1614-1617
+  if (Node->getTI().Sets.size() == 0) {
+    OS << "default(";
+    return;
+  }
----------------
abidmalikwaterloo wrote:
> ABataev wrote:
> > Is this correct? Just `default(` is expected to be printed?
> This function is called from `StmtPrinter::PrintOMPExecutableDirective ` in `StmtPrinter.CPP`. The enclosed Directive and the second matching brace will be printed. See line # 670 in `StmtPrinter.CPP`
The problem is this doesn't make sense. For one, StmtPrinter calls this function conditionally, so you might end up with a lonely closing parenthesis. Also, why would we split the parenthesis into two places?


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1679
+  OS << ": ";
+}
+
----------------
I'm assuming we already have a printer for trait selectors, no? Doesn't `OMPTraitInfo::print` do this already and actually handle scores?


================
Comment at: clang/lib/AST/StmtPrinter.cpp:667
+      	OMPWhenClause *WhenClause = dyn_cast<OMPWhenClause>(Clause);
+      	if (WhenClause!=nullptr){
+      	  if (WhenClause->getDKind() != llvm::omp::OMPD_unknown){
----------------



================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2544
+    // based on the context selector score                      
+    SmallVector<std::pair<unsigned, llvm::APInt>> SortedCluases;
+    
----------------
typo


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2567
+      	Clauses.push_back(Clause);
+      }// end of if statement	
+      
----------------
We don't use comments like these.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2607
+   // Parsing the OpenMP region which will take the
+   // metadirective
+   
----------------
This seems to be copied from somewhere. It is unclear why a metadirective needs to have an associated openmp region. 


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2474
                                tok::annot_pragma_openmp_end);
+                               
     while (Tok.isNot(tok::annot_pragma_openmp_end)) {
----------------
ABataev wrote:
> Restore original code here
There are plenty of formatting changes in this file and elsewhere. Please retore the original code or properly clang-format the patch.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4704
   }
+  
   return SR;
----------------
Still unrelated newlines.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:7390
+        case TraitSelector::device_arch:{
+          bool archMatch = false;
+          for (const OMPTraitProperty &Property : Selector.Properties){
----------------
Code style, ArchMatch.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:7439
+      }
+    }
+
----------------
Why does this perform partial trait matching? We should have code for this. Also, the logic for device_arch and vendor (which is most what there is), is not what we want. Reuse the existing matching logic instead.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:4795
+  		
+  CD->setBody(Res->getCapturedStmt());   
   RD->completeDefinition();
----------------
Unrelated. Please go over the patch and avoid these in the future. We have so many comments that point out these things that now the comments make it hard to read/review. 


================
Comment at: clang/test/OpenMP/metadirective_ast_print_new_1.cpp:20
+// CHECK: void bar()
+// CHECK: #pragma omp metadirective when(user={condition(N > 10)}: target teams) default(parallel for)
----------------
I doubt clang format handles the metadirective well but maybe format the code first without the pragma. This is hard to read. Also the other tests. 

You only have ast print tests, what about the rest? Messages (errors/warnings) and codegen?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:378
+    });	
+}
+ 
----------------
This does not sort scores properly and for some reason uses a Map as a intermediate storage.
Instead, please combine this with the function below, which already has a proper score comparator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122255