-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
Fix missing ifdef HPX_SMT_PAUSE #5327
Conversation
Can one of the admins verify this patch? |
retest |
if (k == 0) | ||
{ | ||
HPX_SMT_PAUSE; | ||
} | ||
else | ||
#endif |
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.
Wouldn't it be better to actually define the macro instead of this conditional block?
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 agree. I guess _mm_pause would be the one for intel.
I just followed how it's currently done in other parts of the code:
hpx/libs/core/threading_base/src/execution_agent.cpp
Lines 65 to 70 in c320e11
#if defined(HPX_SMT_PAUSE) | |
else if (k < 16) | |
{ | |
HPX_SMT_PAUSE; | |
} | |
#endif |
hpx/libs/core/execution_base/src/this_thread.cpp
Lines 95 to 97 in c320e11
#if defined(HPX_SMT_PAUSE) | |
HPX_SMT_PAUSE; | |
#else |
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.
Please see https://github.com/STEllAR-GROUP/hpx/pull/5199/files#diff-94191e4d2264ae001231f8f358e44f0a855b33a427e48e41e8df445e5309fe5c as well. That defines HPX_SMT_PAUSE
to be _mm_pause
.
However, while doing those changes I was considering dropping support for ICC. Now that we have someone who seems to use ICC, I'd be curious to hear how hard of a requirement you have to use ICC? Do you find that ICC does a better job in whatever application you're using HPX?
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.
Then that PR should be able to fix this issue as well. The main reason for using ICC in our application (independent of HPX) is better automatic vectorization of some complex for-loops compared to GCC which leads to 2x speedup for that part of the code.
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.
Fair enough, thanks! That's a reasonable reason for using ICC. I'll see how we can keep the ICC CI going then (probably in a reduced form).
Due to the missing definition of
HPX_SMT_PAUSE
incompiler_fence.hpp
, source filespinlock.cpp
cannot be compiled with the Intel compiler.hpx/libs/core/config/include/hpx/config/compiler_fence.hpp
Lines 22 to 26 in c320e11