Page MenuHomePhabricator

[OpenMP] Refactor memory allocation code for easier support for third party memory libraries
AcceptedPublic

Authored by lionkov on Jul 23 2020, 12:39 PM.

Details

Reviewers
jdoerfert
AndreyChurbanov
grokos
Group Reviewers
Restricted Project
Summary

Redesign the memory allocation code to make it easier to plug custom third-party libraries.
Add support for the SICM third-party library.

Diff Detail

Event Timeline

lionkov created this revision.Jul 23 2020, 12:39 PM
lionkov retitled this revision from Refactor OpenMP memory allocation code for easier support for third party memory librariesRedesign the memory allocation code to make it easier to plug custom third-party libraries. Add support for the SICM third-party library. to Refactor OpenMP memory allocation code for easier support for third party memory libraries.Jul 23 2020, 12:39 PM
lionkov edited the summary of this revision. (Show Details)

Assuming I read this right, it should be split into two patches. First, move the memkind logic into the thirdparty folder. Second, introduce the SICM support. WDYT?

openmp/runtime/src/kmp_runtime.cpp
531

We should use LIBOMP_USE_SICM to avoid calling any "sicm" related functions. This avoids the need for mock functions.

jdoerfert retitled this revision from Refactor OpenMP memory allocation code for easier support for third party memory libraries to [OpenMP] Refactor memory allocation code for easier support for third party memory libraries.Jul 23 2020, 12:46 PM
jdoerfert added reviewers: AndreyChurbanov, grokos.
lionkov updated this revision to Diff 280985.Jul 27 2020, 11:13 AM

Removed the SICM support, will post that as separate patch once this one is accepted.

Any progress on reviewing this?

I'm the wrong person to review this. @AndreyChurbanov

openmp/runtime/src/kmp.h
966

What is aux? Could you provide a doxygen style comment for all new members please.

openmp/runtime/src/kmp_alloc.cpp
1321

Nit: No braces, or braces for the else.

1341

Are these comments left over or should they explain something?

lionkov updated this revision to Diff 284846.Aug 11 2020, 12:21 PM

Fixed minor issues reported by Johannes.

lionkov marked 4 inline comments as done.Aug 11 2020, 12:22 PM

Could you please provide full diff so that more context is available around the changes?

Could you please provide full diff so that more context is available around the changes?

Sorry, this is my first patch to llvm. What dows "full diff" mean?

Could you please provide full diff so that more context is available around the changes?

Sorry, this is my first patch to llvm. What dows "full diff" mean?

No problemo, we have documentation :)

https://llvm.org/docs/Contributing.html#how-to-submit-a-patch

Other than few formatting changes the instructions from the link didn't provide any information about "full diff". Is that related to arcanist somehow? I can try to figure out how to use it if that'll help. :)

Other than few formatting changes the instructions from the link didn't provide any information about "full diff". Is that related to arcanist somehow? I can try to figure out how to use it if that'll help. :)

I figured if I give you the link to the start of "how to submit a patch" you would follow the links from there. My bad: https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

Other than few formatting changes the instructions from the link didn't provide any information about "full diff". Is that related to arcanist somehow? I can try to figure out how to use it if that'll help. :)

I figured if I give you the link to the start of "how to submit a patch" you would follow the links from there. My bad: https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

That's exactly the instructions I followed to submit my patch. If it is lacking some details, I would like to know how to fix it.

Other than few formatting changes the instructions from the link didn't provide any information about "full diff". Is that related to arcanist somehow? I can try to figure out how to use it if that'll help. :)

I figured if I give you the link to the start of "how to submit a patch" you would follow the links from there. My bad: https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

That's exactly the instructions I followed to submit my patch. If it is lacking some details, I would like to know how to fix it.

The key difference is:
git diff # gives you short patch
git diff -U999999 # gives you full patch

The full patch should have complete source, as opposed to 3 lines above and below the changes for the short patch.

lionkov updated this revision to Diff 285073.Aug 12 2020, 7:14 AM

Other than few formatting changes the instructions from the link didn't provide any information about "full diff". Is that related to arcanist somehow? I can try to figure out how to use it if that'll help. :)

I figured if I give you the link to the start of "how to submit a patch" you would follow the links from there. My bad: https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

That's exactly the instructions I followed to submit my patch. If it is lacking some details, I would like to know how to fix it.

The key difference is:
git diff # gives you short patch
git diff -U999999 # gives you full patch

The full patch should have complete source, as opposed to 3 lines above and below the changes for the short patch.

Ugh, sorry I missed that! Fixed.

The key difference is:
git diff # gives you short patch
git diff -U999999 # gives you full patch

The full patch should have complete source, as opposed to 3 lines above and below the changes for the short patch.

Ugh, sorry I missed that! Fixed.

Not sure what do you mean by "fixed", I still don't see a context around the changes (probably you didn't upload full diff, even if you got it).
Anyway I tried to review the patch as it is. But please upload the full diff once you address the comments.

openmp/runtime/src/kmp.h
923

This enum is never used. Better to remove the dead code.

openmp/runtime/src/kmp_alloc.cpp
1236

Why do you use abort_fb for all standard allocators? The null_fb looks more logical to me for all but the omp_null_allocator. Users can check the result and perform some actions if allocation fails. Abort does not give them a chance.

1356

CCAST is no-op here, please remove. RCAST(kmp_allocator_t *, allocator) will work fine.

1399

CCAST is no-op here as well, please eliminate it. This is historical artifact, as initially the parameter type was "const omp_allocator_handle_t". Now it is not const any more.

1432

Wrong intentation. Add 2 more spaces.

1482

Poor indentation. Please fix.

openmp/runtime/src/kmp_settings.cpp
3312

Either use

sizeof(kmp_standard_allocators) / sizeof(kmp_standard_allocators[0])

instead of 9,or add some comment so that we don't miss this place in case number of standard allocators is changed in future.

3317

I'd guess __kmp_allocator_names[i] should be used instead of "omp_high_bw_mem_alloc" here.

openmp/runtime/src/thirdparty/memkind/kmp_memkind.cpp
83

I think "sizeof(kmp_standard_allocators) / sizeof(kmp_standard_allocators[0])" is much safer than "9" as upper bound.
As the number of standard allocators can be changed in future.

92

#endif misplaced. Move it inside __kmp_init_memkind function.

97

I'd remove this check and move the TRACE under if(h_memkind).

As theoretically kmp_mk_check can be non-NULL when h_memkind is NULL and memkind is not used (e.g. when expression in "if" on line 63 evaluates to false). Your code nullifies h_memkind, but does not nullify kmp_mk_check, kmp_mk_alloc, kmp_mk_free, mk_default, in case something goes wrong.

lionkov updated this revision to Diff 286077.Aug 17 2020, 10:43 AM

Changes suggested by the reviewers.

lionkov marked 3 inline comments as done.Aug 17 2020, 10:43 AM

Any progress on this change?

AndreyChurbanov accepted this revision.Sep 11 2020, 1:31 PM

Looks good, besides a few formatting nits.

openmp/runtime/src/kmp.h
925

Two underscores in large_cap member looks like a typo.

openmp/runtime/src/kmp_alloc.cpp
1397–1403

Too long line, needs formatting.

1417

Too long line, please use clang-format to format the change.

1422–1434

Please replace tabs with 8 spaces in this code block.

openmp/runtime/src/kmp_settings.cpp
3312–3320

Too long line, please format.

This revision is now accepted and ready to land.Sep 11 2020, 1:31 PM
lionkov updated this revision to Diff 291685.Sep 14 2020, 2:25 PM

changes suggested by the reviewers