search

libocca / occa

Portable and vendor neutral framework for parallel programming on heterogeneous platforms.
https://libocca.org
MIT License
396 stars 86 forks source link

Add kernel/include_occa and kernel/link_occa properties #675

Closed deukhyun-cha closed 1 year ago

deukhyun-cha commented 1 year ago

Add kernel/include_occa and kernel/link_occa properties to control including and linking occa into kernels. This PR is a replica of the original PR #603 by @SFrijters with very minor cleanup (removed changes already merged in, and squash the original commits), just in case if there would be any further changes required.

Original description by @SFrijters in the PR was:

**Recently I have been debugging a segmentation fault in our code that seems to have been caused by transitive dependencies of occa. During this process I noticed that all kernels get linked to libocca and occa headers are #included at the top of the kernel code. However, none of our kernels require anything from inside occa, so I think it would be nice to be able to control this. Not linking in occa speeds things up a little bit and potentially prevents unexpected behaviour.

This PR keeps the default behaviour as-is, but makes linking to occa, and #include(i)ng the occa headers and namespace opt-out per kernel. This seems to work well in our code at this moment. Is this something that seems generally useful enough to include?

Open questions: should we do some bikeshedding on the flag name and where should this new option be documented? And I'm not quite sure if the markdown files in docs should just be edited directly or if there is some kind of automation for that? Remark: I have not been able to test with HIP, but the change looks straightforward enough (famous last words).

I also noticed some inconsistencies in how the kernelHash is calculated, based on what goes into the compiler invocation so this is a separate fix commit. It could be taken out of this PR if requested.**

Please refer the original PR #603 for other discussions has been made.

codecov[bot] commented 1 year ago

Codecov Report

Merging #675 (1648d09) into development (4c46a28) will increase coverage by 0.01%. The diff coverage is 93.33%.

:exclamation: Current head 1648d09 differs from pull request most recent head 8794698. Consider uploading reports for the commit 8794698 to get more accurate results

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/libocca/occa/pull/675/graphs/tree.svg?width=650&height=150&src=pr&token=doaG4c84wZ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libocca)](https://app.codecov.io/gh/libocca/occa/pull/675?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libocca) ```diff @@ Coverage Diff @@ ## development #675 +/- ## =============================================== + Coverage 76.24% 76.25% +0.01% =============================================== Files 291 291 Lines 18806 18819 +13 =============================================== + Hits 14339 14351 +12 - Misses 4467 4468 +1 ``` | [Impacted Files](https://app.codecov.io/gh/libocca/occa/pull/675?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libocca) | Coverage Δ | | |---|---|---| | [src/occa/internal/modes/serial/device.cpp](https://app.codecov.io/gh/libocca/occa/pull/675?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libocca#diff-c3JjL29jY2EvaW50ZXJuYWwvbW9kZXMvc2VyaWFsL2RldmljZS5jcHA=) | `88.70% <88.88%> (+0.01%)` | :arrow_up: | | [src/loops/typelessForLoop.cpp](https://app.codecov.io/gh/libocca/occa/pull/675?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libocca#diff-c3JjL2xvb3BzL3R5cGVsZXNzRm9yTG9vcC5jcHA=) | `98.30% <100.00%> (+0.02%)` | :arrow_up: | | [src/occa/internal/lang/modes/serial.cpp](https://app.codecov.io/gh/libocca/occa/pull/675?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libocca#diff-c3JjL29jY2EvaW50ZXJuYWwvbGFuZy9tb2Rlcy9zZXJpYWwuY3Bw) | `30.04% <100.00%> (+1.04%)` | :arrow_up: |
noelchalmers commented 1 year ago

I've tested this in some OCCA projects. This certainly will be a breaking change for many, as we'll need to be explicit about including headers like <cmath>, but I think that's workable if we're clear about it in release notes.

amikstcyr commented 1 year ago

Any decision was made concerning this PR?

deukhyun-cha commented 1 year ago

Thanks Kris. now the opencl and sycl backends are updated.