Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add kmp_* wrapper for gomp environment #79

Merged
merged 11 commits into from
May 28, 2024
Merged

Add kmp_* wrapper for gomp environment #79

merged 11 commits into from
May 28, 2024

Conversation

Menooker
Copy link
Contributor

omp dialect of MLIR is lowered to kmp_* function calls to libomp (and libiomp). These symbols are not provided by gomp. This will force the end-users to use clang instead of g++ to compile the runtime. To avoid this issue, as is recommended by the openmp team, we provide a wrapper over the standard omp primitives to implement the libomp symbols based on gomp. Currently limited features are supported, including static schedule omp-for and omp-section.

The wrapper will be disabled when the runtime is linked with libomp/libiomp.

@@ -0,0 +1,15 @@
find_package(OpenMP REQUIRED)

if ("iomp" IN_LIST OpenMP_C_LIB_NAMES OR "omp" IN_LIST OpenMP_C_LIB_NAMES OR "omp5" IN_LIST OpenMP_C_LIB_NAMES)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this match "libiomp", "libiomp5", "libomp", "libomp5"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should. :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should. :)

If so, then the "omp" IN_LIST OpenMP_C_LIB_NAMES will also match libgomp? And this is not what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then the "omp" IN_LIST OpenMP_C_LIB_NAMES will also match libgomp

No, I suppose. libgomp has the name gomp in OpenMP_C_LIB_NAMES. IN_LIST of cmake performs a full-name search, instead of finding substr.

"libiomp", "libiomp5", "libomp", "libomp5"

It should only match iomp omp omp5

lib/gc/ExecutionEngine/CPURuntime/Parallel.cpp Outdated Show resolved Hide resolved
lib/gc/ExecutionEngine/CPURuntime/Parallel.cpp Outdated Show resolved Hide resolved
Comment on lines +115 to +123
if (incr == 1) {
trip_count = *pupper - *plower + 1;
} else if (incr == -1) {
trip_count = *plower - *pupper + 1;
} else if (incr > 0) {
// upper-lower can exceed the limit of signed type
trip_count = (UT)(*pupper - *plower) / incr + 1;
} else {
trip_count = (UT)(*plower - *pupper) / (-incr) + 1;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we can combine all these into a single statement trip_count = (UT)(*pupper - *plower) / incr + 1;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is copied and simplified from libomp of LLVM. I would like to keep the original code if possible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, then it would be better to add a reference link here?

@Menooker
Copy link
Contributor Author

ping... any further comments?

Comment on lines +115 to +123
if (incr == 1) {
trip_count = *pupper - *plower + 1;
} else if (incr == -1) {
trip_count = *plower - *pupper + 1;
} else if (incr > 0) {
// upper-lower can exceed the limit of signed type
trip_count = (UT)(*pupper - *plower) / incr + 1;
} else {
trip_count = (UT)(*plower - *pupper) / (-incr) + 1;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, then it would be better to add a reference link here?

@ciyongch
Copy link

@ZhennanQin @kurapov-peter any comments from your side? We need to merge this as there's some PR dependency now.


extern "C" {
int gc_runtime_keep_alive = 0;
void gc_arrive_at_barrier(barrier_t *b, barrier_idle_func idle_func,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the usage scenario for the barrier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not currently used for now. But we will introduce cpu barrier ops in CPURuntime in near future.


WIDTH: int = 80
intel_license: list[str] = [
intel_license: List[str] = [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WangJialei-A Hi, in my local python (3.8.10), list[str] is not allowed. I have updated the script here.

@kurapov-peter kurapov-peter merged commit 05fcc76 into main May 28, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants