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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
