-
Notifications
You must be signed in to change notification settings - Fork 6
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 benchmark #41
Add benchmark #41
Conversation
Results on Icelake
|
Build errors on HIP
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two minor comments for my understanding.
set(KokkosFFT_VERSION_MAJOR 0) | ||
set(KokkosFFT_VERSION_MINOR 0) | ||
set(KokkosFFT_VERSION_PATCH 00) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're set to release a beta, you should probably put a 0.1.0 or something I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. As well as DDC, I am planning to begin with 0.0.00.
math(EXPR KOKKOSFFT_VERSION "${KokkosFFT_VERSION_MAJOR} * 10000 + ${KokkosFFT_VERSION_MINOR} * 100 + ${KokkosFFT_VERSION_PATCH}") | ||
math(EXPR KOKKOSFFT_VERSION_MAJOR "${KOKKOSFFT_VERSION} / 10000") | ||
math(EXPR KOKKOSFFT_VERSION_MINOR "${KOKKOSFFT_VERSION} / 100 % 100") | ||
math(EXPR KOKKOSFFT_VERSION_PATCH "${KOKKOSFFT_VERSION} % 100") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the use of the KOKKOSFFT_VERSION variable. Could you be a little more explicit please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KOKKOSFFT_VERSION
is a temporary variable to convert version variables into integers. What we would really like to have in integers are KOKKOSFFT_VERSION_MAJOR
, KOKKOSFFT_VERSION_MINOR
and KOKKOSFFT_VERSION_PATCH
. These are used to replace the placeholders in c++ codes that are used to show version info in benchmark.
Is it OK for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented, this will be done in the fix of #32
For the moment, need to focus on docs and thread capability.
@acalloo First of all, thank you for your reviews. |
@pzehner I found the errors in hip build, which says
For some reason, build tests completed. There seems to be a fix for that after rocm5.3.0+. |
I will merge this one and update CMake in other PR. |
In this PR, I have added micro bench based on googlebenchmark. Need to implement more practical examples.