Skip to content

(closes #2856) Configurable intrinsics available on device#3013

Merged
arporter merged 23 commits into
masterfrom
2856_configurable_intr_available_on_device
Jun 25, 2025
Merged

(closes #2856) Configurable intrinsics available on device#3013
arporter merged 23 commits into
masterfrom
2856_configurable_intr_available_on_device

Conversation

@sergisiso

@sergisiso sergisiso commented Jun 3, 2025

Copy link
Copy Markdown
Collaborator

It turns out that the performance degradation of NEMO BENCH with the new compiler is not due to the new excluded files, but due to the new excluded intrinsic REAL. This is only necessary for the reproducible run, so I introduced a "device-id" option to some of the transformation that mark regions of code for the GPU to decide what intrinsics to permit using this device string.

At the moment there is only a nvfortran-all (what we should use for performance results) and a nvfortran-repr that we should use for numerically reproducible results. But this now can be easily extensible for other compiler/architectures/feature-sets, e.g. amd/cray/gnu.

Sine I am building NEMO BENCH twice, I thought it would also be useful to keep the profiling hooks in one of them.

The final result for NEMO bench is faster than it use to be even before excluding REAL, but this probably because now I use optimising flags for the build to get the performance timings.

@codecov

codecov Bot commented Jun 3, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.91%. Comparing base (54578cf) to head (96035a8).
Report is 24 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3013   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files         365      365           
  Lines       52043    52066   +23     
=======================================
+ Hits        51999    52022   +23     
  Misses         44       44           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sergisiso sergisiso requested a review from arporter June 5, 2025 09:21
@sergisiso sergisiso self-assigned this Jun 5, 2025
@sergisiso

Copy link
Copy Markdown
Collaborator Author

@arporter This is ready for a first review

@sergisiso

Copy link
Copy Markdown
Collaborator Author

@arporter This is ready for another look

@arporter arporter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very, very nearly there. There's just a bit of confusion in the error message - you have it (almost) right in transformations.py but in omp_target_trans.py you always say "default device".

Comment thread src/psyclone/psyir/transformations/omp_target_trans.py Outdated
Comment thread src/psyclone/tests/psyir/transformations/kernel_transformation_test.py Outdated
Comment thread src/psyclone/transformations.py Outdated
Comment thread src/psyclone/transformations.py Outdated
@sergisiso

Copy link
Copy Markdown
Collaborator Author

@arporter I fixed the error messages, this is ready for another look

arporter
arporter previously approved these changes Jun 23, 2025

@arporter arporter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All looks good now thanks Sergi.
I've fired off the integration tests just to be on the safe side.

@arporter arporter dismissed their stale review June 23, 2025 12:43

The NEMO 5 integration test failed with a Signal 11.

@arporter

Copy link
Copy Markdown
Member

Sorry Sergi but the NEMOv5 integration test failed with a Signal 11 :-(

@sergisiso

Copy link
Copy Markdown
Collaborator Author

@arporter I click re-run and the test succeeded, I think it was a pre-existing issue that does not manifest every time. Also the file that failed does not have any of the intrinsics configured by this PR.

@arporter arporter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks Sergi. I'll proceed to merge.
Doc test failure was a link-check problem with the GOcean page so nothing to do with this PR.

@arporter arporter merged commit 15c9455 into master Jun 25, 2025
11 of 12 checks passed
@arporter arporter deleted the 2856_configurable_intr_available_on_device branch June 25, 2025 11:27
arporter added a commit that referenced this pull request Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants