[NFCI][SYCL][Graph] Cleanup after enable_shared_from_this for queue_impl
#18748
Conversation
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
aelovikov-intel
force-pushed
the
queue_impl
branch
from
1dc5c88 to
ec363dd
Compare
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
force-pushed
the
queue_impl
branch
from
ec363dd to
9b58b66
Compare
aelovikov-intel
changed the title
enable_shared_from_this for queue_implenable_shared_from_this for queue_impl
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
force-pushed
the
queue_impl
branch
from
9b58b66 to
74782fe
Compare
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| addQueue(const std::shared_ptr<sycl::detail::queue_impl> &RecordingQueue) { | ||
| MRecordingQueues.insert(RecordingQueue); | ||
| } | ||
| void addQueue(sycl::detail::queue_impl &RecordingQueue); |
There was a problem hiding this comment.
I had to move this and the ones below into the graph_impl.cpp because queue_impl is incomplete at this point. queue_impl.hpp/graph_impl.hpp include each other (which is totally wrong but not the subject of this PR) and untangling that would be a task that I'm not willing to take on, at least not now.
Another alternative would be to make these templates:
template <typename QueueImplTy = queue_impl>
void foo(QueueImplTy &Q)
to make weak_from_this() template-type-dependent and delay the requirement for a complete type until foo is instantiated (vs defined as of now).
Accepting std::weak_ptr<queue_impl> would also work (and some pre-existing methods do just that already) but I find it lacking in expressiveness - it doesn't communicate expectations about nullptr possibility of if the argument can be in already expired state.
There was a problem hiding this comment.
I had to move this and the ones below into the graph_impl.cpp because queue_impl is incomplete at this point. queue_impl.hpp/graph_impl.hpp include each other (which is totally wrong but not the subject of this PR) and untangling that would be a task that I'm not willing to take on, at least not now.
Wasn't aware of the bidirectional include of queue_impl.hpp and graph_impl.hpp, thanks for pointing that out. We've been wanting to split graph_impl hpp/cpp up into smaller files as it's gotten quite large, so made a note on our internal ticket for that to remove this bidirectional include.
I think moving these functions into the cpp file is fine though.
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Initially started in intel#18830 Subsequent PRs before this final one: intel#18794 intel#18834 intel#18748
|
@intel/llvm-reviewers-runtime (@cperkinsintel especially), ping. |
Continuation of the refactoring in intel#18715 intel#18748
foo(const std::shared_ptr<queue_impl> &)doesn't provide any information about possiblenullptrarguments. It could provide information about possibility of creating a copy if used sparsely, but when entire codebase passes all objects like this it's meaningless too.Instead of that, the WIP refactoring across the entire codebase is to pass by raw ptr/ref (depending on the possibility of
nullptrvalue) and extending lifetimes with explicitshared_from_this().This PR is limited to
graph_impl.hppinterfaces acceptingqueue_impl, clean up after #18715.