Update EKAT#8088
Conversation
amametjanov
left a comment
amametjanov
left a comment
There was a problem hiding this comment.
I don't know about the other changes, but updates to
- cmake/EkatSetCompilerFlags.cmake
- extern/kokkos
are good.
|
@bartgol Are the other changes in EKAT ok to pull to master, or is there anything that needs to wait? There's fails in SP which I'll investigate. |
|
Everything should be tested and safe to use. There have been a few mods in the That said, I am not using single precision in hommexx+sacado, so there may be some issue there. If you figure out what it is, ping me on any ninja fix PR in ekat (or assign me to it). |
|
@tcclevenger I just reopened a PR in E3SM-Project/kokkos. I think we need to thread carefully with unmerged PRs when dealing with submodules... Edit: I think all is good. The kokkos pr was into the e3sm-master branch, and got merged, so we're good as long as we keep pointing to e3sm-master. |
tcclevenger
force-pushed
the
tcclevenger/update_ekat_aurora_fixes
branch
from
7d75e33 to
b056263
Compare
| result = max(beta*result,static_cast<const ScalarOut&>(alpha*newVal)); | ||
| result = max(beta*result,alpha*newVal); | ||
| break; | ||
| case CombineMode::Min: | ||
| result = min(beta*result,static_cast<const ScalarOut&>(alpha*newVal)); |
There was a problem hiding this comment.
@bartgol Not sure why these were needed, but after updating ekat that caused these error messages
E3SM/components/eamxx/src/share/util/eamxx_combine_ops.hpp:73:32: error: invalid ‘static_cast’ from type ‘std::enable_if_t<true, ekat::Pack<double, 16> >’ {aka ‘ekat::Pack<double, 16>’} to type ‘const ekat::Pack<float, 16>&’
73 | result = max(beta*result,static_cast<const ScalarOut&>(alpha*newVal));
I think the cast can just be safely removed?
There was a problem hiding this comment.
I think this was to allow the compiler to build when ScalarIn and ScalarOut are different, like in this case. When doing max(double,float), assuming the compiler managed to do it, I would expect it to promot float->double, while I wanted all ops to happen in whatever precision the output is, so I casted to the output scalar.
I thought this would be from IO using single-precision for output, but the eamxx FM storing in double precision, but I see that the error comes from the single precision build. I don't think we should have any double in a SP build...
Perhaps we could adjust the static cast so that it doesn't have references; if coeff is already of type ScalarOut (possibly with ref/cv qualifiers), then it's compiled away, otherwise a ScalarOut is constructed from the result).
There was a problem hiding this comment.
Ah, the error was in combine ops unit test, which does
combine<Update>(two,x,2.0,1.0);while two and x are packs of Real. We should replace the hard-coded doubles like 1.0 with Real(1.0) (or simply with integers, as those should work), and reinstate the cast (without the reference though, as that doesn't play well with templates).
There was a problem hiding this comment.
Also, if you do that, there some leftover debugging stuff in the combine ops unit tests. Can you prune it?
There was a problem hiding this comment.
Yep, I think that's it. Thanks!
tcclevenger
force-pushed
the
tcclevenger/update_ekat_aurora_fixes
branch
from
eee2d6c to
6adef18
Compare
|
@bartgol This look good to merge to you? Even though it changes Kokkos, it's a single line so I don't think we need to do next testing? |
bartgol
left a comment
bartgol
left a comment
There was a problem hiding this comment.
EKAT changes are only impacting eamxx, and the one-liner change in kokkos should not affect any existing run, so I am in favor of merging.
Motivation: incorporating changes for Aurora and Chrysalis oneapi-ifx compiler. Other changes included. [BFB]
3 participants
Motivation: incorporating changes for Aurora and Chrysalis oneapi-ifx compiler. Other changes included.
[BFB]