Refactor bdy imp3#561
Conversation
|
vern_comb_after_3n_16T_run2.txt:| bdy_imp3 | 23.37296 | 22.793 | 24.303 | 23.37296 | 22.793 | 24.303 | 421.66104 | 480 | 3.54854 | 0.04869 | Its way way worse doing it this way. Hmmmm. Not as easy to workout |
|
So its not the removal of the vector loop. It's the size of the loop (and or possibly the dynamic schedule). Splitting it back into '3' loops, each with their own OMP coverage (one_over_v is effectively a loop over an array which is i_len / threads) has brought the performance back into some normalcy. vern_comb_after_3n_16T_run1.txt:| bdy_imp3 | 0.69154 | 0.676 | 0.735 | 0.69154 | 0.676 | 0.735 | 633.28846 | 480 | 0.10804 | 0.00144 | It also highlights that blocking was still beneficial. We should cross compare another datapoint. |
|
Latest changes with no waits are broadly speaking very similar now. vern_comb_after_3n_16T_run1.txt:| bdy_imp3 | 0.61492 | 0.599 | 0.654 | 0.61492 | 0.599 | 0.654 | 630.74225 | 480 | 0.09658 | 0.00128 | |
Benjamin Went (MetBenjaminWent)
requested a review
from Adrian Lock (Adrian-Lock)
as a code owner
Benjamin Went (MetBenjaminWent)
requested a review
from Hacka Fett (christophermaynard)
Adrian Lock (Adrian-Lock)
left a comment
Adrian Lock (Adrian-Lock)
left a comment
There was a problem hiding this comment.
No problems with this at all - actually improves the readability of the code too. Thanks, Adrian
|
Graph of Data: |
3 participants
PR Summary
Longer term we need to be using PSyclone on all Physics source for Hardware portable source.
Whilst it's functionality is being expanded, as edge cases are found, there is also an argument for refactoring code to make this easier for PSyclone.
In this file case, PSyclone is seeing the
temp(l)array and defaulting it to OMP shared which causes issues (a crash).We are adding a method to PSyclone to allow us to force privatise variables as required.
STFC are also exploring their dependency analysis so that this is better in the future.
Further analysis of the loop indicates that the primary need for being written in this way, a internal call to one over v is to use an specific hardware optimisation library, with limited blocking by threads (which was found to be the best method with the original source when optimising in #314).
This library call isn't required currently on our systems with CCE. Performance testing below shows that we are effectively the same without the call.
Also the long term intent of PSyclone is to be the method for handling alot of this.
Furthermore in the context of protable code, GPU porting will struggle with this loop.
Therefore we can make this a nicer loop for PSyclone today for CPU, without sacrificing performance on our EX systems with CCE, improve the loop structure for GPU porting, and move a hardware optimisation into the correct conceptual home, PSyclone.
Before an After timings for these changes. I've only done a single run, and I expect some of the results are in the noise, but given they are so close I think the change effect is low.
Sci/Tech Reviewer: Hacka Fett (@christophermaynard)
Code Reviewer: allynt
Linked Issues:
Issue: #560
Umbrella Work: #106
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - refactor-bdy-imp3/run7
Suite Information
Task Information
✅ succeeded tasks - 51
Test Suite Results - lfric_apps - refactor-bdy-imp3/run8
Suite Information
Task Information
❌ failed tasks - 1
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review