[PATCH] D71847: [OpenMP][Part 1] Reusable OpenMP context/traits handling
Jon Chesterfield via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 13 18:14:10 PST 2020
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
More information about the llvm-commits
mailing list