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

compiler: Keep -qopenmp by default after icx 2023.2 #2164

Merged
merged 3 commits into from
Jul 21, 2023

Conversation

georgebisbas
Copy link
Contributor

@georgebisbas georgebisbas commented Jul 20, 2023

Passes locally
After icx 2023.2.0, reductions should are passing

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #2164 (9fea3ca) into master (a1da72e) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #2164      +/-   ##
==========================================
- Coverage   87.04%   87.04%   -0.01%     
==========================================
  Files         223      223              
  Lines       39957    39962       +5     
  Branches     7295     7300       +5     
==========================================
+ Hits        34781    34784       +3     
- Misses       4597     4598       +1     
- Partials      579      580       +1     
Impacted Files Coverage Δ
devito/arch/compiler.py 38.55% <0.00%> (-0.17%) ⬇️
tests/test_mpi.py 98.87% <100.00%> (+<0.01%) ⬆️

if language == 'openmp':
# Since OneAPI 2023.2.0 (clang17 underneath), we can use '-qopenmp'
# for all our tests
if self.version >= Version('17.0.0') and language == 'openmp':
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this doing the opposite of what the comment says?

also, if you are to change this (maybe not, maybe I misunderstood...), could you drop "for all our tests" which doesn't mean much? what you should have said is that earlier versions have an OpenMP bug. The bug is in their compilers, it doesn't have anything to do with our tests (I mean, not directly)

Copy link
Contributor Author

@georgebisbas georgebisbas Jul 21, 2023

Choose a reason for hiding this comment

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

Ah sorry, you are right thanks!

@mloubout mloubout merged commit e076d36 into master Jul 21, 2023
32 checks passed
@mloubout mloubout deleted the icx2023.2-qopenmp-fix branch July 21, 2023 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants