[Openmp-commits] [PATCH] D83281: [OpenMP] Allow traits for the OpenMP context selector `isa`

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:7599
+  /// (which accept anything) and (later) extensions.
+  StringRef RawString{};
 };
----------------
No need for the default initializer here


================
Comment at: clang/include/clang/AST/OpenMPClause.h:7658
+/// Clang specific specialization of the OMPContext to lookup target features.
+struct TargetOMPContext : public llvm::omp::OMPContext {
+
----------------
`final` and virtual default destructor?


================
Comment at: clang/include/clang/AST/OpenMPClause.h:7665
+  /// See llvm::omp::OMPContext::matchesISATrait
+  bool matchesISATrait(StringRef RawString) const;
+
----------------
`override`?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1826
+
+    std::function<void(StringRef)> DiagUnknownTrait = [&](StringRef ISATrait) {
+      // TODO Track the selector locations in a way that is accessible here to
----------------
Seems to me, `SourceLocation` is usually captured by value.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5894
   ASTContext &Context = getASTContext();
-  OMPContext OMPCtx(getLangOpts().OpenMPIsDevice,
-                    Context.getTargetInfo().getTriple());
+  std::function<void(StringRef)> DiagUnknownTrait = [&](StringRef ISATrait) {
+    // TODO Track the selector locations in a way that is accessible here to
----------------
`CE` can be captured by value


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:205
+    if (Property == TraitProperty::device_isa___ANY)
+      IsActiveTrait = std::all_of(
+          VMI.ISATraits.begin(), VMI.ISATraits.end(),
----------------
`llvm::all_of`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83281