jdoerfert added a comment. This looks close to an OpenMP 5.0 implementation. I left comments inlined.
We need tests that show how non-selected alternatives *do not* impact the program. As an example, a template instantiation inside of a non-selected alternative is not actually performed. We also need test with ill-formed metadirectives. ================ 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 ---------------- no `!`. The default clause doesn't need to be in the end either. ================ Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1952 - STMT_MS_DEPENDENT_EXISTS, // MSDependentExistsStmt - EXPR_LAMBDA, // LambdaExpr STMT_COROUTINE_BODY, ---------------- Unrelated. ================ Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:10211 + return; + } + ---------------- Can you explain this, this seems odd to me. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:2186 + // parse and get condition expression to pass to the When clause + parseOMPContextSelectors(Loc, TI); + ---------------- Usually you would check the return value in case we later actually propagate errors while parsing the context selector. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:2193 + Diag(Tok, diag::warn_pragma_expected_colon) << "when clause"; + return Directive; + } ---------------- If we give up it should be an error, I think. If we issue a warning we just pretend the colon was there afterwards. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:2204 + ConsumeAnyToken(); + } + // Parse ')' ---------------- We have balanced trackers for this that also deal with the fact that there might be a `)` missing. This code will probably crash. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:2216 + TPA.Revert(); + TargetOMPContext OMPCtx(ASTContext, nullptr, nullptr); + int BestIdx = getBestWhenMatchForContext(VMIs, OMPCtx); ---------------- Add a TODO that `nullptr` should be replaced as per the use in `Sema::ActOnOpenMPCall`. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:2237 + continue; + } + ---------------- Use a BalancedDelimiterTracker. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:2254 + // Parse ':' + ConsumeAnyToken(); + } ---------------- If you warn and continue above you need to check for `:` here again. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:2260 + DirKind = parseOpenMPDirectiveKind(*this); + ConsumeToken(); + if (DirKind != OMPD_unknown) { ---------------- What is this token? We have skipped this part before so we need to validate it is what we expect it to be. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:2264 + Actions.StartOpenMPDSABlock(DirKind, DirName, Actions.getCurScope(), + Loc); + int paren = 0; ---------------- Should we not go back to the original code handling "directives" instead? This looks like it is copied here. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:2317 + ConsumeAnnotationToken(); + } + } else { ---------------- Same as below, change the order. Also, the "skipping" part is always the same, put it in a helper function or lambda. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:2324 + ConsumeAnnotationToken(); + } + break; ---------------- Move the smaller case first and use an early exit. That will reduce the indention of the larger case by 1. ================ Comment at: clang/test/OpenMP/metadirective_construct.cpp:12 + when(construct = {"target"} \ + : distribute parallel for) default() + for (int i = 0; i < N; i++) ---------------- Since when does the `construct` trait work? I'm confused. ================ 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 ---------------- no -fopenmp-targets please. ================ 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 ---------------- Can we run this for all configurations of the metadirective so we can actually see it will pick the right one, not the first? ================ 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 ---------------- Leftover. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:337 + const SmallVectorImpl<VariantMatchInfo> &VMIs, const OMPContext &Ctx, + SmallVectorImpl<unsigned> *OrderedMatch) { + ---------------- This looks like a clone of `getBestVariantMatchForContext` with an extra unused argument. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:400 + return BestVMIIdx; +}*/ + ---------------- Leftover. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91944/new/ https://reviews.llvm.org/D91944 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits