-
Notifications
You must be signed in to change notification settings - Fork 433
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
fill_random without exceution space instance should fence #6658
Conversation
This is an execution space fence not a global fence, this won't be blocking the whole device, seems different from what is done in the Kokkos Kernels PR? |
744057f
to
a40ca43
Compare
a40ca43
to
391daef
Compare
Kokkos::fence( | ||
"fill_random: fence before since no execution space instance provided"); | ||
typename ViewType::execution_space exec; | ||
fill_random(exec, a, g, begin, end); | ||
exec.fence( | ||
"fill_random: fence after since no execution space instance provided"); |
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.
Once again I'd like to propose a wrapper for this kind of change.
This pattern is really coming over and over again in the whole code base, and to me it's silly not willing to have something that helps:
- ensure the message for the fence is coherent before and after the statement
- ensure the behavior is always
Kokkos::fence
(global) before the statement and *space.fence` (device specific) after the statement.
template <typename execution_space>
class Fencer
{
public:
Fencer(std::string msg_) : msg(std::move(msg_))
{
Kokkos::fence(msg + ": fence before since no execution space instance provided");
}
~Fencer()
{
space.fence(msg + ": fence after since no execution space instance provided");
}
public:
execution_space space {};
private:
std::string msg;
};
template <typename execution_space>
void my_function(const execution_space&, std::ostream& out)
{
out << __PRETTY_FUNCTION__ << std::endl;
}
void my_function(std::ostream& out)
{
out << __PRETTY_FUNCTION__ << ": enter" << std::endl;
my_function(Fencer<execution_space>("my_function").space, out);
out << __PRETTY_FUNCTION__ << ": exit" << std::endl;
}
TEST(Kokkos, fencer)
{
std::cout << "BEGIN OF TEST" << std::endl;
my_function(std::cout);
std::cout << "END OF TEST" << std::endl;
}
that would give
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[ RUN ] Kokkos.fencer
BEGIN OF TEST
void HELM::tests::utils::my_function(std::ostream&): enter
KokkosP: Executing fence on device 16777216 with unique execution identifier 0
KokkosP: my_function: fence before since no execution space instance provided
KokkosP: Execution of fence 0 is completed.
KokkosP: Executing fence on device 0 with unique execution identifier 1
KokkosP: my_function: fence before since no execution space instance provided
KokkosP: Execution of fence 1 is completed.
void HELM::tests::utils::my_function(const execution_space&, std::ostream&) [with execution_space = Kokkos::OpenMP; std::ostream = std::basic_ostream<char>]
KokkosP: Executing fence on device 16777217 with unique execution identifier 2
KokkosP: my_function: fence after since no execution space instance provided
KokkosP: Execution of fence 2 is completed.
void HELM::tests::utils::my_function(std::ostream&): exit
END OF TEST
[ OK ] Kokkos.fencer (0 ms)
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (0 ms total)
[ PASSED ] 1 test.
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.
Note that
void my_function(std::ostream& out) {
my_function(Fencer<execution_space>("my_function").space, out);
}
can also be written as
void my_function(std::ostream& out)
{
Fencer<execution_space> fencer("msg");
my_function(fencer.space, out);
}
(looks like a scope guarded thing)
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.
Take the discussion to its own issue if you want to gauge for appetite for it.
I am unconvinced.
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.
FWIF if someone throws an exception (ugh) in between, at least we will fence properly with this class.
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.
Discussion moved to #6716.
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.
Looks good to me, though I do like @romintomasetti 's suggestion I think that shouldn't block this PR.
Ignoring failures |
Related to kokkos/kokkos-kernels#2066. We were seeing erroneous timings in benchmarks that didn't fence after calling
fill_random
without execution space instance.Following our general syntax, the overload without execution space instance should fence.