[PATCH] D71847: [OpenMP][Part 1] Reusable OpenMP context/traits handling

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield requested changes to this revision.
JonChesterfield added a comment.
This revision now requires changes to proceed.

Mostly minor comments. You've got a lot of mileage out of x macros. I really like the unit tests generated from the same .def as the source.

I think there's a bug in isSubset where you're comparing iterators but should be comparing values, not sure why the tests miss that though. Marking as request changes / what I've missed for that line.

Thanks!



================
Comment at: llvm/include/llvm/ADT/SetOperations.h:72
+bool set_is_subset(const S1Ty &S1, const S2Ty &S2) {
+  for (auto &It : S1)
+    if (!S2.count(It))
----------------
I wonder if a size comparison and early return would be profitable here.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:264
+  OMP_TRAIT_PROPERTY(TraitSet##_##TraitSelector##_##Name,                   \
+                     TraitSet, TraitSet##_##TraitSelector, #Name)
+
----------------
clang-format


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:87
+
+template <typename T> static bool isSubset(ArrayRef<T> C0, ArrayRef<T> C1) {
+  if (C0.size() > C1.size())
----------------
Probably warrants a comment that it checks for C0 is a subset of C1. Not sure there's a strong convention on argument order here.

Also a comment that it requires the arrays to have been sorted, assuming an assertion is too expensive


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:95
+      return false;
+    if (It0 == It1) {
+      ++It0;
----------------
This looks strange - missing dereferences?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:114
+                           const VariantMatchInfo &VMI1) {
+  // If all required traits are a srict subset and the ordered vectors storing
+  // the construct traits, we say it is a strict subset. Note that the latter
----------------
srict -> strict


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:224
+  assert(NoConstructTraits == ConstructMatches.size() &&
+         "Missmatch in the construct traits!");
+  for (TraitProperty Property : VMI.ConstructTraits) {
----------------
Missmatch => mismatch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71847