Page MenuHomePhabricator

[OpenMP][Part 1] Reusable OpenMP context/traits handling
ClosedPublic

Authored by jdoerfert on Dec 23 2019, 1:54 PM.

Details

Summary

This is the first of multiple parts to make OpenMP context/trait
handling reusable and generic. This patch was originally part of D71830
but with the unit tests it can be tested independently.

This patch implements an almost complete handling of OpenMP
contexts/traits such that we can reuse most of the logic in Flang
through the OMPContext.{h,cpp} in llvm/Frontend/OpenMP.

All but construct SIMD specifiers, e.g., inbranch, and the device ISA
selector are define in llvm/lib/Frontend/OpenMP/OMPKinds.def. From
these definitions we generate the enum classes TraitSet,
TraitSelector, and TraitProperty as well as conversion and helper
functions in llvm/lib/Frontend/OpenMP/OMPContext.{h,cpp}.

The OpenMP context is now an explicit object (see struct OMPContext).
This is in anticipation of construct traits that need to be tracked. The
OpenMP context, as well as the VariantMatchInfo, are basically made up
of a set of active or respectively required traits, e.g., 'host', and an
ordered container of constructs which allows duplication. Matching and
scoring is kept as generic as possible to allow easy extension in the
future.

Diff Detail

Event Timeline

jdoerfert created this revision.Dec 23 2019, 1:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2019, 1:54 PM

Unit tests: pass. 60972 tests passed, 0 failed and 726 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

JonChesterfield requested changes to this revision.Jan 13 2020, 6:11 PM

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!

llvm/include/llvm/ADT/SetOperations.h
72

I wonder if a size comparison and early return would be profitable here.

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
264

clang-format

llvm/lib/Frontend/OpenMP/OMPContext.cpp
87

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

95

This looks strange - missing dereferences?

114

srict -> strict

224

Missmatch => mismatch

This revision now requires changes to proceed.Jan 13 2020, 6:11 PM
jdoerfert updated this revision to Diff 241612.Jan 30 2020, 5:15 PM
jdoerfert marked 6 inline comments as done.

Addressed review comments

Unit tests: fail. 62360 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: fail. clang-tidy found 0 errors and 6 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

JonChesterfield accepted this revision.Feb 1 2020, 3:04 AM

Nice use of expensive_checks.

This revision is now accepted and ready to land.Feb 1 2020, 3:04 AM
This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Mar 13 2020, 5:00 PM
rnk added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPContext.h
120

I'd suggest that you think about different data structures here. I noticed that this instantiation of SmallSet, which wraps std::set, takes ~20ms, and happens in every file that includes OMPContext.h.

Can you use a BitVector or std::bitset instead?

jdoerfert marked an inline comment as done.Mar 13 2020, 5:42 PM
jdoerfert added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPContext.h
120

TBH, I don't understand why this takes any time at all as it is just a type. Do you mean having std::set in the type is already slow?

I still run test but what do you think of D76170 ?

rnk added inline comments.Mar 14 2020, 8:54 AM
llvm/include/llvm/Frontend/OpenMP/OMPContext.h
120

Basically, yes, all the time is spent instantiating std::set<>::insert. This is a set of enumerators, though, which are densely packed integers with some known fixed maximum, so a bit vector seems like a more natural representation.