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 kernel/include_occa and kernel/link_occa properties #675

Merged
merged 1 commit into from Jul 21, 2023

Conversation

deukhyun-cha
Copy link
Contributor

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
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

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

❗ 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

@@               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 Coverage Δ
src/occa/internal/modes/serial/device.cpp 88.70% <88.88%> (+0.01%) ⬆️
src/loops/typelessForLoop.cpp 98.30% <100.00%> (+0.02%) ⬆️
src/occa/internal/lang/modes/serial.cpp 30.04% <100.00%> (+1.04%) ⬆️

@noelchalmers
Copy link
Contributor

noelchalmers commented Apr 23, 2023

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
Copy link
Contributor

Any decision was made concerning this PR?

Copy link
Member

@kris-rowe kris-rowe left a comment

Choose a reason for hiding this comment

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

A minor revision: the kernel hashing and compilation in the OpenCL and SYCL backends should be updated like the CUDA and HIP backends.

@deukhyun-cha
Copy link
Contributor Author

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

@kris-rowe kris-rowe merged commit 14b570d into libocca:development Jul 21, 2023
6 checks passed
kris-rowe pushed a commit that referenced this pull request Sep 12, 2023
@deukhyun-cha deukhyun-cha deleted the require-occa branch November 9, 2023 15:34
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